irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

According to Cortex-A7 MPCore TRM, ch8.3.1, Distributor register summary, GICD_ITARGETSRn:

The register that contains the SGI and PPI interrupts is read-only and the value is implementation defined. For Cortex-A7 configurations with only one processor, these registers are RAZ/WI.

So, the GICD_ITARGETSR[0..7] is read-only, and the value is 0 for cortex-a7 single core platform if the SoC is cortex-a7 mpcore version. So the cupmask from gic_get_cpumask is 0.

At ARM Generic Interrupt Controller Architecture version 2.0, ch4.3.15 Software Generated Interrupt Register, GICD_SGIR, The distributor will process the requested SGI according to register TargetListFilter and CPUTargetList. At current gic code, it takes TargetListFilter as 0b00, and forward the interrupt to cpumask (variable map at gic_raise_softirq) getting from gic_get_cpumask. but cpumask is 0 according to above explanation for cortex-a7 single core platform, so, both TargetListFilter and CPUTargetList are 0, and the distributor does not forward the interrupt to any CPU interface according to gic documentation, then the SGI can't be occurred.

We have found this problem at nxp imx6ul platform, which is a cortex-a7 single core platform, the irq work (triggered by SGI at ARM) can't be occurred which is needed for cpufreq, then the system has failed to boot and reboot [1].

In this commit, we set TargetListFilter as 0b10 to fix this problem, it forwards the interrupt only to CPU0 and only cortex-a7 single core platform uses this setting currently.

Cc Russell King <[email protected]>

Cc Shawn Guo <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Jason Liu <[email protected]>

Cc Anson Huang <[email protected]>

Cc Frank Li <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Ping Bai <[email protected]>

Signed-off-by Peter Chen <[email protected]>

---
 drivers/irqchip/irq-gic.c | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

drivers/irqchip/irq-gic.c +36 -3

index c2cab57..625ae6d 100644
@@ -764,6 +764,23 @@ static int gic_pm_init(struct gic_chip_data *gic)
 #endif
 
 #ifdef CONFIG_SMP
+static void gic_raise_softirq_itself(const struct cpumask *mask,
+				     unsigned int irq)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&irq_controller_lock, flags);
+
+	/*
+	 * Forward the interrupt only to the CPU interface of the processor
+	 * that requested the interrupt.
+	 */
+	writel_relaxed(0x2000000 | irq, gic_data_dist_base(&gic_data[0])
+					+ GIC_DIST_SOFTINT);
+
+	raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
+}
+
 static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 {
 	int cpu;

@@ -1162,9 +1179,6 @@ static int __init __gic_init_bases(struct gic_chip_data *gic,
 		 */
 		for (i = 0; i < NR_GIC_CPU_IF; i++)
 			gic_cpu_map[i] = 0xff;
-#ifdef CONFIG_SMP
-		set_smp_cross_call(gic_raise_softirq);
-#endif
 		cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_GIC_STARTING,
 					  "AP_IRQ_GIC_STARTING",
 					  gic_starting_cpu, NULL);

@@ -1336,6 +1350,16 @@ static void __init gic_of_setup_kvm_info(struct device_node *node)
 	gic_set_kvm_info(&gic_v2_kvm_info);
 }
 
+static bool __init gic_is_a7_singlecore(struct gic_chip_data *gic,
+					struct device_node *node)
+{
+	if (!of_device_is_compatible(node, "arm,cortex-a7-gic"))
+		return false;
+
+	return !(readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_CTR) & 0xe0);
+
+}
+
 int __init
 gic_of_init(struct device_node *node, struct device_node *parent)
 {

@@ -1367,6 +1391,15 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 		return ret;
 	}
 
+	if (gic == &gic_data[0]) {
+#ifdef CONFIG_SMP
+		if (gic_is_a7_singlecore(gic, node))
+			set_smp_cross_call(gic_raise_softirq_itself);
+		else
+			set_smp_cross_call(gic_raise_softirq);
+#endif
+	}
+
 	if (!gic_cnt) {
 		gic_init_physaddr(node);
 		gic_of_setup_kvm_info(node);

Signature · 7 lines

1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 29 lines

According to Cortex-A7 MPCore TRM, ch8.3.1, Distributor register summary,
GICD_ITARGETSRn:

The register that contains the SGI and PPI interrupts is read-only and the value is implementation defined. For Cortex-A7 configurations with only one processor, these registers are RAZ/WI.

So, the GICD_ITARGETSR[0..7] is read-only, and the value is 0 for cortex-a7 single core platform if the SoC is cortex-a7 mpcore version. So the cupmask from gic_get_cpumask is 0.

At ARM Generic Interrupt Controller Architecture version 2.0, ch4.3.15 Software Generated Interrupt Register, GICD_SGIR, The distributor will process the requested SGI according to register TargetListFilter and CPUTargetList. At current gic code, it takes TargetListFilter as 0b00, and forward the interrupt to cpumask (variable map at gic_raise_softirq) getting from gic_get_cpumask. but cpumask is 0 according to above explanation for cortex-a7 single core platform, so, both TargetListFilter and CPUTargetList are 0, and the distributor does not forward the interrupt to any CPU interface according to gic documentation, then the SGI can't be occurred.

We have found this problem at nxp imx6ul platform, which is a cortex-a7 single core platform, the irq work (triggered by SGI at ARM) can't be occurred which is needed for cpufreq, then the system has failed to boot and reboot [1].

Quoted 3 lines

In this commit, we set TargetListFilter as 0b10 to fix this problem, it forwards the interrupt only to CPU0 and only cortex-a7 single core platform uses this setting currently.

This is a generic property of the GIC architecture in UP systems, and is not specific to Cortex-A7. So checking for Cortex-A7 specifically doesn't solve the problem.

As Russell pointed out in [2], this is a generic infrastructure problem, and there are other systems where HW might not have a self-IPI.

I note that core code already
handles that in some cases, e.g. in generic_exec_single where we just disable interrupts and run the work locally rather than sending a self-IPI.

How/where exactly is this self-IPI raised? Can we follow the example of generic_exec_single there?

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-May/430545.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-May/430849.html

drivers/irqchip/irq-gic.c static int __init __gic_init_bases(struct gic_chip_data

Quoted 43 lines

Cc Russell King <[email protected]>

Cc Shawn Guo <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Jason Liu <[email protected]>

Cc Anson Huang <[email protected]>

Cc Frank Li <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Ping Bai <[email protected]>

Signed-off-by Peter Chen <[email protected]>

---
 drivers/irqchip/irq-gic.c | 39
++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

drivers/irqchip/irq-gic.c +16 -0

index c2cab57..625ae6d 100644
@@ -764,6 +764,23 @@ static int gic_pm_init(struct gic_chip_data *gic)
 #endif
 
 #ifdef CONFIG_SMP
+static void gic_raise_softirq_itself(const struct cpumask *mask,
+				     unsigned int irq)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&irq_controller_lock, flags);
+
+	/*
+	 * Forward the interrupt only to the CPU interface of the processor
+	 * that requested the interrupt.
+	 */
+	writel_relaxed(0x2000000 | irq, gic_data_dist_base(&gic_data[0])
+					+ GIC_DIST_SOFTINT);
+
+	raw_spin_unlock_irqrestore(&irq_controller_lock, flags); }
+
 static void gic_raise_softirq(const struct cpumask *mask, unsigned
 int irq)  {
 	int cpu;

@@ -1162,9 +1179,6 @@ static int __init __gic_init_bases(struct gic_chip_data

*gic,

drivers/irqchip/irq-gic.c static void __init gic_of_setup_kvm_info(struct

Quoted 10 lines

drivers/irqchip/irq-gic.c +0 -3

 		 */
 		for (i = 0; i < NR_GIC_CPU_IF; i++)
 			gic_cpu_map[i] = 0xff;
-#ifdef CONFIG_SMP
-		set_smp_cross_call(gic_raise_softirq);
-#endif
 		cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_GIC_STARTING,
 					  "AP_IRQ_GIC_STARTING",
 					  gic_starting_cpu, NULL);

@@ -1336,6 +1350,16 @@ static void __init gic_of_setup_kvm_info(struct

device_node *node)

drivers/irqchip/irq-gic.c gic_of_init(struct device_node *node, struct

Quoted 17 lines

drivers/irqchip/irq-gic.c +11 -0

 	gic_set_kvm_info(&gic_v2_kvm_info);
 }
 
+static bool __init gic_is_a7_singlecore(struct gic_chip_data *gic,
+					struct device_node *node)
+{
+	if (!of_device_is_compatible(node, "arm,cortex-a7-gic"))
+		return false;
+
+	return !(readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_CTR) &
+0xe0);
+
+}
+
 int __init
 gic_of_init(struct device_node *node, struct device_node *parent)  {

@@ -1367,6 +1391,15 @@ gic_of_init(struct device_node *node, struct

device_node *parent)

Quoted 24 lines

drivers/irqchip/irq-gic.c +9 -1

 		return ret;
 	}
 
+	if (gic == &gic_data[0]) {
+#ifdef CONFIG_SMP
+		if (gic_is_a7_singlecore(gic, node))
+			set_smp_cross_call(gic_raise_softirq_itself);
+		else
+			set_smp_cross_call(gic_raise_softirq);
+#endif
+	}
+
 	if (!gic_cnt) {
 		gic_init_physaddr(node);
 		gic_of_setup_kvm_info(node);
--

1.9.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 36 lines · Peter Chen

Quoted 32 lines

According to Cortex-A7 MPCore TRM, ch8.3.1, Distributor register summary,
GICD_ITARGETSRn:

The register that contains the SGI and PPI interrupts is read-only and the value is implementation defined. For Cortex-A7 configurations with only one processor, these registers are RAZ/WI.

So, the GICD_ITARGETSR[0..7] is read-only, and the value is 0 for cortex-a7 single core platform if the SoC is cortex-a7 mpcore version. So the cupmask from gic_get_cpumask is 0.

At ARM Generic Interrupt Controller Architecture version 2.0, ch4.3.15 Software Generated Interrupt Register, GICD_SGIR, The distributor will process the requested SGI according to register TargetListFilter and CPUTargetList. At current gic code, it takes TargetListFilter as 0b00, and forward the interrupt to cpumask (variable map at gic_raise_softirq) getting from gic_get_cpumask. but cpumask is 0 according to above explanation for cortex-a7 single core platform, so, both TargetListFilter and CPUTargetList are 0, and the distributor does not forward the interrupt to any CPU interface according to gic documentation, then the SGI can't be occurred.

We have found this problem at nxp imx6ul platform, which is a cortex-a7 single core platform, the irq work (triggered by SGI at ARM) can't be occurred which is needed for cpufreq, then the system has failed to boot and reboot [1].

In this commit, we set TargetListFilter as 0b10 to fix this problem, it forwards the interrupt only to CPU0 and only cortex-a7 single core platform uses this setting currently.

Quoted 2 lines

This is a generic property of the GIC architecture in UP systems, and is not specific to Cortex-A7. So checking for Cortex-A7 specifically doesn't solve the problem.

It is a SMP system, the is_smp returns true due to MPIDR is 0x80000000. This platform is MPcore, just the cpu number is one.

Current kernel considers the hardware is IPI capable if is_smp is true, see arch_irq_work_has_interrupt(). I think I should add additional condition is_smp == true.

Quoted 2 lines

As Russell pointed out in [2], this is a generic infrastructure problem, and there are other systems where HW might not have a self-IPI.

If the HW is UP, it is no problem. The irq work will not trigger IPI.

Quoted 6 lines

I note that core code already
handles that in some cases, e.g. in generic_exec_single where we just disable interrupts and run the work locally rather than sending a self-IPI.

How/where exactly is this self-IPI raised? Can we follow the example of generic_exec_single there?

In my case, the cpufreq uses irq work, irq work tries to trigger IPI. See sugov_update_commit-> irq_work_queue.

imx6ul is MPcore system, just single core. If ARM considers MPcore system has IPI capabilities, and documentation is correct, then it is probably gic code's issue.

Quoted 2 lines

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-May/430545.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-May/430849.html

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

In this commit, we set TargetListFilter as 0b10 to fix this problem, it forwards the interrupt only to CPU0 and only cortex-a7 single core platform uses this setting currently.

This is a generic property of the GIC architecture in UP systems, and is not specific to Cortex-A7. So checking for Cortex-A7 specifically doesn't solve the problem.

Quoted 2 lines

It is a SMP system, the is_smp returns true due to MPIDR is 0x80000000. This platform is MPcore, just the cpu number is one.

Apologies, I see the distinction now. The CPU claims to have the multiprocessing extensions, and to be part of a multiprocessor system (despite the latter not being the case). The GIC is not a multiprocessor implementation as defined in the GIC architecture.

Quoted 3 lines

Current kernel considers the hardware is IPI capable if is_smp is true, see arch_irq_work_has_interrupt(). I think I should add additional condition is_smp == true.

I see that for arm64 we have:

static inline bool arch_irq_work_has_interrupt(void)

{
	return !!__smp_cross_call;
}

Could we do similarly for ARM, and ony register gic_raise_softirq if we have non-zero SGI targets?

If I've understood correctly, that would make things behave as they do for UP on you system.

Quoted 6 lines

In my case, the cpufreq uses irq work, irq work tries to trigger IPI. See sugov_update_commit-> irq_work_queue.

imx6ul is MPcore system, just single core. If ARM considers MPcore system has IPI capabilities, and documentation is correct, then it is probably gic code's issue.

If self-IPI is necessary, then this would be up to the GIC code to solve.

For that case, it would be nicer if we could detect whether this was necessary based on the GIC registers alone. That way we handle the various ways this can be integrated, aren't totally relient on the DT, work in VMs, etc.

Quoted 12 lines · Mark Rutland

I see that for arm64 we have:

static inline bool arch_irq_work_has_interrupt(void)

{
	return !!__smp_cross_call;
}

Could we do similarly for ARM, and ony register gic_raise_softirq if we have non-zero SGI targets?

If I've understood correctly, that would make things behave as they do for UP on you system.

Quoted 7 lines

If self-IPI is necessary, then this would be up to the GIC code to solve.

For that case, it would be nicer if we could detect whether this was necessary based on the GIC registers alone. That way we handle the various ways this can be integrated, aren't totally relient on the DT, work in VMs, etc.

How we can detect IPI capabilities based on GIC register?

Quoted 13 lines · Peter Chen

Quoted 12 lines · Mark Rutland

I see that for arm64 we have:

static inline bool arch_irq_work_has_interrupt(void)

{
	return !!__smp_cross_call;
}

Could we do similarly for ARM, and ony register gic_raise_softirq if we have non-zero SGI targets?

If I've understood correctly, that would make things behave as they do for UP on you system.

[...]

Quoted 9 lines

Quoted 7 lines

If self-IPI is necessary, then this would be up to the GIC code to solve.

For that case, it would be nicer if we could detect whether this was necessary based on the GIC registers alone. That way we handle the various ways this can be integrated, aren't totally relient on the DT, work in VMs, etc.

How we can detect IPI capabilities based on GIC register?

Check the mask associated with SGIs, as we do for gic_get_cpumask(). If this is zero, we have a non-multiprocessor GIC (or one that's otherwise broken), and can't do SGI in the usual way.

However, it only makes sense to do this if self-IPI is truly a necessity. Given there are other interrupt controllers that can't do self-IPI, avoiding self-IPI in general would be a better strategy, avoiding churn in each and every driver...

Quoted 31 lines · On Mon, 8 Aug 2016 14:48:42 +0100 Mark Rutland <[email protected]> wrote:

Quoted 13 lines · Peter Chen

Quoted 12 lines · Mark Rutland

I see that for arm64 we have:

static inline bool arch_irq_work_has_interrupt(void)

{
	return !!__smp_cross_call;
}

Could we do similarly for ARM, and ony register gic_raise_softirq if we have non-zero SGI targets?

If I've understood correctly, that would make things behave as they do for UP on you system.

[...]

Quoted 9 lines

Quoted 7 lines

If self-IPI is necessary, then this would be up to the GIC code to solve.

For that case, it would be nicer if we could detect whether this was necessary based on the GIC registers alone. That way we handle the various ways this can be integrated, aren't totally relient on the DT, work in VMs, etc.

How we can detect IPI capabilities based on GIC register?

Check the mask associated with SGIs, as we do for gic_get_cpumask(). If this is zero, we have a non-multiprocessor GIC (or one that's otherwise broken), and can't do SGI in the usual way.

Quoted 4 lines

However, it only makes sense to do this if self-IPI is truly a necessity. Given there are other interrupt controllers that can't do self-IPI, avoiding self-IPI in general would be a better strategy, avoiding churn in each and every driver...

Indeed. And I won't take such a patch until all other avenues have been explored, including fixing core code if required...

Quoted 39 lines · Marc Zyngier

Quoted 35 lines · On Mon, 8 Aug 2016 14:48:42 +0100 Mark Rutland <[email protected]> wrote:

Quoted 13 lines · Peter Chen

Quoted 12 lines · Mark Rutland

I see that for arm64 we have:

static inline bool arch_irq_work_has_interrupt(void)

{
	return !!__smp_cross_call;
}

Could we do similarly for ARM, and ony register gic_raise_softirq if we have non-zero SGI targets?

If I've understood correctly, that would make things behave as they do for UP on you system.

[...]

Quoted 9 lines

Quoted 7 lines

If self-IPI is necessary, then this would be up to the GIC code to solve.

For that case, it would be nicer if we could detect whether this was necessary based on the GIC registers alone. That way we handle the various ways this can be integrated, aren't totally relient on the DT, work in VMs, etc.

How we can detect IPI capabilities based on GIC register?

Check the mask associated with SGIs, as we do for gic_get_cpumask(). If this is zero, we have a non-multiprocessor GIC (or one that's otherwise broken), and can't do SGI in the usual way.

However, it only makes sense to do this if self-IPI is truly a necessity. Given there are other interrupt controllers that can't do self-IPI, avoiding self-IPI in general would be a better strategy, avoiding churn in each and every driver...

Quoted 3 lines

Indeed. And I won't take such a patch until all other avenues have been explored, including fixing core code if required...

Ok, it seems both you and Mark agree with disable IPI for GIC who has only self-IPI capability (GICD_ITARGETSR0 to GICD_ITARGETSR7 are all zero), right?

But even we do that, we still have problem that the callers for smp_cross_call don't know well if the platform has IPI capability. Eg, IRQ work considers the SMP system has IPI capability, but it is not a must in this case (Cortex-A7 MPcore version, but cpu number is one). It will cause NULL pointer dereference problem as __smp_cross_call is NULL, and we need to make below change to let it work:

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

index 937c892..276bd94 100644

From 9be5fc16f10e4d32a8ad3d70db50d2dfb96f70a1 Mon Sep 17 00:00:00 2001

Quoted 44 lines · On Tue, 9 Aug 2016 11:46:13 +0800 Peter Chen <[email protected]> wrote:

Quoted 42 lines · Marc Zyngier

Quoted 27 lines · On Mon, 8 Aug 2016 14:48:42 +0100 Mark Rutland <[email protected]> wrote:

Quoted 13 lines · Peter Chen

Quoted 12 lines · Mark Rutland

I see that for arm64 we have:

static inline bool arch_irq_work_has_interrupt(void)

{
	return !!__smp_cross_call;
}

Could we do similarly for ARM, and ony register gic_raise_softirq if we have non-zero SGI targets?

If I've understood correctly, that would make things behave as they do for UP on you system.

[...]

Quoted 9 lines

Quoted 7 lines

If self-IPI is necessary, then this would be up to the GIC code to solve.

For that case, it would be nicer if we could detect whether this was necessary based on the GIC registers alone. That way we handle the various ways this can be integrated, aren't totally relient on the DT, work in VMs, etc.

How we can detect IPI capabilities based on GIC register?

Quoted 8 lines

Check the mask associated with SGIs, as we do for gic_get_cpumask(). If this is zero, we have a non-multiprocessor GIC (or one that's otherwise broken), and can't do SGI in the usual way.

However, it only makes sense to do this if self-IPI is truly a necessity. Given there are other interrupt controllers that can't do self-IPI, avoiding self-IPI in general would be a better strategy, avoiding churn in each and every driver...

Indeed. And I won't take such a patch until all other avenues have been explored, including fixing core code if required...

Quoted 3 lines

Ok, it seems both you and Mark agree with disable IPI for GIC who has only self-IPI capability (GICD_ITARGETSR0 to GICD_ITARGETSR7 are all zero), right?

Not necessarily. This can be seen a latency improvement, compared to the timer method which should be the fallback.

Quoted 13 lines

But even we do that, we still have problem that the callers for smp_cross_call don't know well if the platform has IPI capability. Eg, IRQ work considers the SMP system has IPI capability, but it is not a must in this case (Cortex-A7 MPcore version, but cpu number is one). It will cause NULL pointer dereference problem as __smp_cross_call is NULL, and we need to make below change to let it work:

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

index 937c892..276bd94 100644

arch/arm/kernel/smp.c static const char *ipi_types[NR_IPI] __tracepoint_string = { · line 492

Quoted 7 lines

arch/arm/kernel/smp.c +2 -1

@@ -487,7 +487,8 @@ static const char *ipi_types[NR_IPI] __tracepoint_string = {
 static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
 {
 	trace_ipi_raise_rcuidle(target, ipi_types[ipinr]);
-	__smp_cross_call(target, ipinr);
+	if (__smp_cross_call)
+		__smp_cross_call(target, ipinr);
 }

I came up with a slightly different approach, which is to have arch_irq_work_has_interrupt() to check for an IPI-capable system:

From 9be5fc16f10e4d32a8ad3d70db50d2dfb96f70a1 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
Date: Tue, 9 Aug 2016 06:04:12 +0100
Subject: [PATCH] ARM: irq_work: Do not attempt to IPI on non IPI-capable HW

Not all of the ARM HW is IPI capable (i.e. most of the non-SMP systems). Unfortunately, some systems do advertise being SMP capable, even if they have a single core and do not define a cross call method. In this case, irq_work_queue dies as arch_irq_work_has_interrupt() fails to detect this particular case.

Let's redefine arch_irq_work_has_interrupt() to actually check if we're IPI capable instead of simply being SMP. This sidesteps the issue entierely.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm/include/asm/irq_work.h | 2 +-
 arch/arm/include/asm/smp_plat.h | 2 ++
 arch/arm/kernel/smp.c           | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

arch/arm/include/asm/irq_work.h +1 -1

index 712d03e..025420b 100644
@@ -5,7 +5,7 @@
 
 static inline bool arch_irq_work_has_interrupt(void)
 {
-	return is_smp();
+	return !!__smp_cross_call;
 }
 
 #endif /* _ASM_ARM_IRQ_WORK_H */

arch/arm/include/asm/smp_plat.h +2 -0

index f908071..ffee073 100644
@@ -26,6 +26,8 @@ static inline bool is_smp(void)
 #endif
 }
 
+extern void (*__smp_cross_call)(const struct cpumask *, unsigned int);
+
 /**
  * smp_cpuid_part() - return part id for a given cpu
  * @cpu:	logical cpu id.

arch/arm/kernel/smp.c +1 -1

index 8615216..f9d771f 100644
@@ -465,7 +465,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 	}
 }
 
-static void (*__smp_cross_call)(const struct cpumask *, unsigned int);
+void (*__smp_cross_call)(const struct cpumask *, unsigned int);
 
 void __init set_smp_cross_call(void (*fn)(const struct cpumask *, unsigned int))
 {

Signature · 10 lines

2.8.1

Does it work for you? We could then add self-IPI as a further
optimization.

Thanks,

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

Quoted 51 lines · Marc Zyngier

Quoted 47 lines · On Tue, 9 Aug 2016 11:46:13 +0800 Peter Chen <[email protected]> wrote:

Quoted 34 lines · Marc Zyngier

Quoted 31 lines · On Mon, 8 Aug 2016 14:48:42 +0100 Mark Rutland <[email protected]> wrote:

Quoted 13 lines · Peter Chen

Quoted 12 lines · Mark Rutland

I see that for arm64 we have:

static inline bool arch_irq_work_has_interrupt(void)

{
	return !!__smp_cross_call;
}

Could we do similarly for ARM, and ony register gic_raise_softirq if we have non-zero SGI targets?

If I've understood correctly, that would make things behave as they do for UP on you system.

[...]

Quoted 9 lines

Quoted 7 lines

If self-IPI is necessary, then this would be up to the GIC code to solve.

For that case, it would be nicer if we could detect whether this was necessary based on the GIC registers alone. That way we handle the various ways this can be integrated, aren't totally relient on the DT, work in VMs, etc.

How we can detect IPI capabilities based on GIC register?

Check the mask associated with SGIs, as we do for gic_get_cpumask(). If this is zero, we have a non-multiprocessor GIC (or one that's otherwise broken), and can't do SGI in the usual way.

Quoted 8 lines

Quoted 4 lines

However, it only makes sense to do this if self-IPI is truly a necessity. Given there are other interrupt controllers that can't do self-IPI, avoiding self-IPI in general would be a better strategy, avoiding churn in each and every driver...

Indeed. And I won't take such a patch until all other avenues have been explored, including fixing core code if required...

Ok, it seems both you and Mark agree with disable IPI for GIC who has only self-IPI capability (GICD_ITARGETSR0 to GICD_ITARGETSR7 are all zero), right?

Quoted 3 lines

Not necessarily. This can be seen a latency improvement, compared to the timer method which should be the fallback.

Why? Your below patch (I tried too) just fixes NULL pointer issue for

without define smp_cross_call function. But imx6ul is a SMP platform

(all imx6/7 uses the same configuration with both CONFIG_SMP and
CONFIG_SMP_ON_UP are defined), it still defines smp_cross_call.
We still need the changes at gic code.

Besides, if the hardware has IPI capability, but we just disable it to align with UP platforms, is it reasonable?

From ad5c001cb1359799831bbb69f8582cb41dda4248 Mon Sep 17 00:00:00 2001

Quoted 56 lines · On Tue, 9 Aug 2016 13:57:01 +0800 Peter Chen <[email protected]> wrote:

Quoted 54 lines · Marc Zyngier

Quoted 40 lines · On Tue, 9 Aug 2016 11:46:13 +0800 Peter Chen <[email protected]> wrote:

Quoted 39 lines · Marc Zyngier

Quoted 35 lines · On Mon, 8 Aug 2016 14:48:42 +0100 Mark Rutland <[email protected]> wrote:

Quoted 13 lines · Peter Chen

Quoted 12 lines · Mark Rutland

I see that for arm64 we have:

static inline bool arch_irq_work_has_interrupt(void)

{
	return !!__smp_cross_call;
}

Could we do similarly for ARM, and ony register gic_raise_softirq if we have non-zero SGI targets?

If I've understood correctly, that would make things behave as they do for UP on you system.

[...]

Quoted 9 lines

Quoted 7 lines

If self-IPI is necessary, then this would be up to the GIC code to solve.

For that case, it would be nicer if we could detect whether this was necessary based on the GIC registers alone. That way we handle the various ways this can be integrated, aren't totally relient on the DT, work in VMs, etc.

How we can detect IPI capabilities based on GIC register?

Check the mask associated with SGIs, as we do for gic_get_cpumask(). If this is zero, we have a non-multiprocessor GIC (or one that's otherwise broken), and can't do SGI in the usual way.

However, it only makes sense to do this if self-IPI is truly a necessity. Given there are other interrupt controllers that can't do self-IPI, avoiding self-IPI in general would be a better strategy, avoiding churn in each and every driver...

Quoted 7 lines

Quoted 3 lines

Indeed. And I won't take such a patch until all other avenues have been explored, including fixing core code if required...

Ok, it seems both you and Mark agree with disable IPI for GIC who has only self-IPI capability (GICD_ITARGETSR0 to GICD_ITARGETSR7 are all zero), right?

Not necessarily. This can be seen a latency improvement, compared to the timer method which should be the fallback.

Quoted 1 line

Why? Your below patch (I tried too) just fixes NULL pointer issue for

And that's the first issue to solve.

Quoted 1 line

without define smp_cross_call function. But imx6ul is a SMP platform

It is *not* an SMP platform. It may have a SMP-capable core, but that's about it.

From my point, single-core in multiprocessor system is different

Quoted 3 lines

(all imx6/7 uses the same configuration with both CONFIG_SMP and
CONFIG_SMP_ON_UP are defined), it still defines smp_cross_call.
We still need the changes at gic code.

That's a different story. You could simply not register the cross-call on your UP system, and it would just work.

Quoted 2 lines

Besides, if the hardware has IPI capability, but we just disable it to align with UP platforms, is it reasonable?

Again: having a self-IPI on UP is an optimization. Nothing more.

Now, coming back to your original idea, I'm aiming towards something like this:

From ad5c001cb1359799831bbb69f8582cb41dda4248 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
Date: Tue, 9 Aug 2016 07:50:44 +0100
Subject: [PATCH] irqchip/gic: Allow self-SGIs for SMP on UP configurations

On systems where a single CPU is present, the GIC may not support having SGIs delivered to a target list. In that case, we use the self-SGI mechanism to allow the interrupt to be delivered locally.

Signed-off-by Marc Zyngier <[email protected]>

---
 drivers/irqchip/irq-gic.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/irqchip/irq-gic.c +8 -0

index c2cab57..415aa1e 100644
@@ -771,6 +771,13 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 
 	raw_spin_lock_irqsave(&irq_controller_lock, flags);
 
+	if (unlikely(nr_cpu_ids == 1)) {
+		/* Only one CPU? let's do a self-IPI... */
+		writel_relaxed(2 << 24 | irq,
+			       gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
+		goto out;
+	}
+
 	/* Convert our logical CPU mask into a physical one. */
 	for_each_cpu(cpu, mask)
 		map |= gic_cpu_map[cpu];

@@ -784,6 +791,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 	/* this always happens on GIC0 */
 	writel_relaxed(map << 16 | irq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
 
+out:
 	raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
 }
 #endif

Signature · 9 lines

2.8.1

Does it work for you?

Thanks,

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

Quoted 65 lines · Marc Zyngier

Quoted 52 lines · On Tue, 9 Aug 2016 13:57:01 +0800 Peter Chen <[email protected]> wrote:

Quoted 51 lines · Marc Zyngier

Quoted 47 lines · On Tue, 9 Aug 2016 11:46:13 +0800 Peter Chen <[email protected]> wrote:

Quoted 34 lines · Marc Zyngier

Quoted 31 lines · On Mon, 8 Aug 2016 14:48:42 +0100 Mark Rutland <[email protected]> wrote:

Quoted 13 lines · Peter Chen

Quoted 12 lines · Mark Rutland

I see that for arm64 we have:

static inline bool arch_irq_work_has_interrupt(void)

{
	return !!__smp_cross_call;
}

Could we do similarly for ARM, and ony register gic_raise_softirq if we have non-zero SGI targets?

If I've understood correctly, that would make things behave as they do for UP on you system.

[...]

Quoted 9 lines

Quoted 7 lines

If self-IPI is necessary, then this would be up to the GIC code to solve.

For that case, it would be nicer if we could detect whether this was necessary based on the GIC registers alone. That way we handle the various ways this can be integrated, aren't totally relient on the DT, work in VMs, etc.

How we can detect IPI capabilities based on GIC register?

Check the mask associated with SGIs, as we do for gic_get_cpumask(). If this is zero, we have a non-multiprocessor GIC (or one that's otherwise broken), and can't do SGI in the usual way.

Quoted 8 lines

Quoted 4 lines

However, it only makes sense to do this if self-IPI is truly a necessity. Given there are other interrupt controllers that can't do self-IPI, avoiding self-IPI in general would be a better strategy, avoiding churn in each and every driver...

Indeed. And I won't take such a patch until all other avenues have been explored, including fixing core code if required...

Ok, it seems both you and Mark agree with disable IPI for GIC who has only self-IPI capability (GICD_ITARGETSR0 to GICD_ITARGETSR7 are all zero), right?

Quoted 5 lines

Quoted 3 lines

Not necessarily. This can be seen a latency improvement, compared to the timer method which should be the fallback.

Why? Your below patch (I tried too) just fixes NULL pointer issue for

And that's the first issue to solve.

Quoted 1 line

without define smp_cross_call function. But imx6ul is a SMP platform

Quoted 3 lines

It is *not* an SMP platform. It may have a SMP-capable core, but that's about it.

Well. That's what I thought at the beginning, but the kernel takes it is. At __fixup_smp (arch/arm/kernel/head.S), it only checks MPIDR, for MPcore, it is 0x80000000, it means it is Multiprocessing Extensions and Processor is part of a multiprocessor system.

From my point, single-core in multiprocessor system is different with uniprocessor system. The first one can execute SMP instruction, but can't for latter.

Quoted 4 lines

Quoted 3 lines

(all imx6/7 uses the same configuration with both CONFIG_SMP and
CONFIG_SMP_ON_UP are defined), it still defines smp_cross_call.
We still need the changes at gic code.

Quoted 3 lines

That's a different story. You could simply not register the cross-call on your UP system, and it would just work.

Again, it is taken as SMP by kernel, and run SMP instructions, the kernel does not do fixup symbol for it.

Quoted 6 lines

Quoted 3 lines

Besides, if the hardware has IPI capability, but we just disable it to align with UP platforms, is it reasonable?

Again: having a self-IPI on UP is an optimization. Nothing more.

Quoted 3 lines

Now, coming back to your original idea, I'm aiming towards something like this:

Your below patch can work (tested), but why not registering an self-IPI smp_cross_call function for single core, it can avoid judging in code for each IPI calls.

From 23ba8b645d219e333a10224d74fb5d8d75d67de2 Mon Sep 17 00:00:00 2001

Quoted 66 lines · Peter Chen

Quoted 65 lines · Marc Zyngier

Quoted 52 lines · On Tue, 9 Aug 2016 13:57:01 +0800 Peter Chen <[email protected]> wrote:

Quoted 51 lines · Marc Zyngier

Quoted 47 lines · On Tue, 9 Aug 2016 11:46:13 +0800 Peter Chen <[email protected]> wrote:

Quoted 34 lines · Marc Zyngier

Quoted 31 lines · On Mon, 8 Aug 2016 14:48:42 +0100 Mark Rutland <[email protected]> wrote:

Quoted 13 lines · Peter Chen

Quoted 12 lines · Mark Rutland

I see that for arm64 we have:

static inline bool arch_irq_work_has_interrupt(void)

{
	return !!__smp_cross_call;
}

Could we do similarly for ARM, and ony register gic_raise_softirq if we have non-zero SGI targets?

If I've understood correctly, that would make things behave as they do for UP on you system.

[...]

Quoted 9 lines

Quoted 7 lines

If self-IPI is necessary, then this would be up to the GIC code to solve.

For that case, it would be nicer if we could detect whether this was necessary based on the GIC registers alone. That way we handle the various ways this can be integrated, aren't totally relient on the DT, work in VMs, etc.

How we can detect IPI capabilities based on GIC register?

Check the mask associated with SGIs, as we do for gic_get_cpumask(). If this is zero, we have a non-multiprocessor GIC (or one that's otherwise broken), and can't do SGI in the usual way.

Quoted 8 lines

Quoted 4 lines

However, it only makes sense to do this if self-IPI is truly a necessity. Given there are other interrupt controllers that can't do self-IPI, avoiding self-IPI in general would be a better strategy, avoiding churn in each and every driver...

Indeed. And I won't take such a patch until all other avenues have been explored, including fixing core code if required...

Ok, it seems both you and Mark agree with disable IPI for GIC who has only self-IPI capability (GICD_ITARGETSR0 to GICD_ITARGETSR7 are all zero), right?

Quoted 5 lines

Quoted 3 lines

Not necessarily. This can be seen a latency improvement, compared to the timer method which should be the fallback.

Why? Your below patch (I tried too) just fixes NULL pointer issue for

And that's the first issue to solve.

Quoted 1 line

without define smp_cross_call function. But imx6ul is a SMP platform

Quoted 8 lines

Quoted 3 lines

It is *not* an SMP platform. It may have a SMP-capable core, but that's about it.

Well. That's what I thought at the beginning, but the kernel takes it is. At __fixup_smp (arch/arm/kernel/head.S), it only checks MPIDR, for MPcore, it is 0x80000000, it means it is Multiprocessing Extensions and Processor is part of a multiprocessor system.

If it is a multi-processor system, please show me the second core. If you can't, this is a UP system, end of story. It may have the MP extensions (there is no such thing as a non-MP A7), but that doesn't make it an SMP system. Please don't confuse the two things.

Again, you're confusing MP-capable with SMP. Yes, the kernel tends to confuse the two as well because it is not always easy to tell them apart (as you just found). That doesn't mean we can't do a better job separating the two concepts when we have the right level of information (i.e. we know the topology of the system).

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

Besides, if the hardware has IPI capability, but we just disable it to align with UP platforms, is it reasonable?

Again: having a self-IPI on UP is an optimization. Nothing more.

Quoted 7 lines

Quoted 3 lines

Now, coming back to your original idea, I'm aiming towards something like this:

Your below patch can work (tested), but why not registering an self-IPI smp_cross_call function for single core, it can avoid judging in code for each IPI calls.

Because it is an unnecessary complication. If you can demonstrate that this single test is an overhead, then I'll consider making this a separate function. Also, we can move the test out of the lock that protects the CPU map as, by definition, there is nothing to protect, making it even more lightweight than your own approach:

From 23ba8b645d219e333a10224d74fb5d8d75d67de2 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
Date: Tue, 9 Aug 2016 07:50:44 +0100
Subject: [PATCH] irqchip/gic: Allow self-SGIs for SMP on UP configurations

On systems where a single CPU is present, the GIC may not support having SGIs delivered to a target list. In that case, we use the self-SGI mechanism to allow the interrupt to be delivered locally.

Signed-off-by Marc Zyngier <[email protected]>

---
 drivers/irqchip/irq-gic.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/irqchip/irq-gic.c +7 -0

index c2cab57..390fac5 100644
@@ -769,6 +769,13 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 	int cpu;
 	unsigned long flags, map = 0;
 
+	if (unlikely(nr_cpu_ids == 1)) {
+		/* Only one CPU? let's do a self-IPI... */
+		writel_relaxed(2 << 24 | irq,
+			       gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
+		return;
+	}
+
 	raw_spin_lock_irqsave(&irq_controller_lock, flags);
 
 	/* Convert our logical CPU mask into a physical one. */

Also, your patch seems to break the arm64 ACPI support by moving the SMP setup to a DT-specific function (I don't see why this should be DT only anyway).

Your patch can work for my case. Below is objdump for gic_raise_softirq, the code with your patch seems have more instructions.

- With your patch:
00000c44 <gic_raise_softirq>:

     c44:	e1a0c00d 	mov	ip, sp
     c48:	e92ddbf0 	push	{r4, r5, r6, r7, r8, r9, fp, ip, lr, pc}
     c4c:	e24cb004 	sub	fp, ip, #4
     c50:	e59f908c 	ldr	r9, [pc, #140]	; ce4 <gic_raise_softirq+0xa0>
     c54:	e1a06000 	mov	r6, r0
     c58:	e1a07001 	mov	r7, r1
     c5c:	e5993000 	ldr	r3, [r9]
     c60:	e3530001 	cmp	r3, #1
     c64:	0a000019 	beq	cd0 <gic_raise_softirq+0x8c>

Quoted 3 lines · Peter Chen

Your patch can work for my case. Below is objdump for gic_raise_softirq, the code with your patch seems have more instructions.

Well, I've added code to the function, so that's hardly surprising. The important thing is that the hot path is kept relatively fast.

Quoted 12 lines

- With your patch:
00000c44 <gic_raise_softirq>:

     c44:	e1a0c00d 	mov	ip, sp
     c48:	e92ddbf0 	push	{r4, r5, r6, r7, r8, r9, fp, ip, lr, pc}
     c4c:	e24cb004 	sub	fp, ip, #4
     c50:	e59f908c 	ldr	r9, [pc, #140]	; ce4 <gic_raise_softirq+0xa0>
     c54:	e1a06000 	mov	r6, r0
     c58:	e1a07001 	mov	r7, r1
     c5c:	e5993000 	ldr	r3, [r9]
     c60:	e3530001 	cmp	r3, #1
     c64:	0a000019 	beq	cd0 <gic_raise_softirq+0x8c>

So the overhead is two loads, one comparison and a branch. Can you actually measure the difference? If you can, I'd like to see some cycle numbers, not just instruction counts. If you can consistently measure something that is not just noise, we can then turn this into a static key.

From 23ba8b645d219e333a10224d74fb5d8d75d67de2 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
Date: Tue, 9 Aug 2016 07:50:44 +0100
Subject: [PATCH] irqchip/gic: Allow self-SGIs for SMP on UP configurations

On systems where a single CPU is present, the GIC may not support having SGIs delivered to a target list. In that case, we use the self-SGI mechanism to allow the interrupt to be delivered locally.

Signed-off-by Marc Zyngier <[email protected]>

From 23ba8b645d219e333a10224d74fb5d8d75d67de2 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
Date: Tue, 9 Aug 2016 07:50:44 +0100
Subject: [PATCH] irqchip/gic: Allow self-SGIs for SMP on UP configurations

On systems where a single CPU is present, the GIC may not support having SGIs delivered to a target list. In that case, we use the self-SGI mechanism to allow the interrupt to be delivered locally.

Signed-off-by Marc Zyngier <[email protected]>

---
 drivers/irqchip/irq-gic.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/irqchip/irq-gic.c +0 -0

index c2cab57..390fac5 100644
@@ -769,6 +769,13 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
        int cpu;

Hi Marc,

Quoted 20 lines · Marc Zyngier

From 23ba8b645d219e333a10224d74fb5d8d75d67de2 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
Date: Tue, 9 Aug 2016 07:50:44 +0100
Subject: [PATCH] irqchip/gic: Allow self-SGIs for SMP on UP configurations

On systems where a single CPU is present, the GIC may not support having SGIs delivered to a target list. In that case, we use the self-SGI mechanism to allow the interrupt to be delivered locally.

Signed-off-by Marc Zyngier <[email protected]>

---
 drivers/irqchip/irq-gic.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/irqchip/irq-gic.c +0 -0

index c2cab57..390fac5 100644
@@ -769,6 +769,13 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
        int cpu;

drivers/irqchip/irq-gic.c static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) · line 779

Quoted 10 lines

drivers/irqchip/irq-gic.c +7 -0

        unsigned long flags, map = 0;
 
+       if (unlikely(nr_cpu_ids == 1)) {
+               /* Only one CPU? let's do a self-IPI... */
+               writel_relaxed(2 << 24 | irq,
+                              gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
+               return;
+       }
+
        raw_spin_lock_irqsave(&irq_controller_lock, flags);

With this patch applied 'reboot' command works fine, but now after going into suspend mode, it reboots automatically.

# echo mem > /sys/power/state
[  285.299792] PM: Syncing filesystems ... done.
[  285.397549] Freezing user space processes ... (elapsed 0.007 seconds) done.
[  285.412868] Double checking all user space processes after OOM
killer disable... (elapsed 0.000 seconds)
[  285.423597] Freezing remaining freezable tasks ... (elapsed 0.008
seconds) done.
[  285.523882] PM: suspend of devices complete after 78.037 msecs
[  285.529926] PM: suspend devices took 0.090 seconds
[  285.542638] PM: late suspend of devices complete after 7.799 msecs
[  285.557131] PM: noirq suspend of devices complete after 8.105 msecs
[  285.563589] Disabling non-boot CPUs ...

U-Boot 2016.07 (Aug 02 2016 - 17:50:40 -0300)

CPU:   Freescale i.MX6UL rev1.0 at 396 MHz
Reset cause: POR
Board: PICO-IMX6UL-EMMC
DRAM:  256 MiB
MMC:   FSL_SDHC: 0
In:    serial
Out:   serial
Err:   serial
Net:   CPU Net Initialization Failed

No ethernet found.
Hit any key to stop autoboot: 0

Maybe because you now have a pending interrupt that you don't handle, making your WFI exit immediately? Sorry, but without more information, it is pretty hard to guess what's happening.

Tested the same kernel on a imx6ul-evk and 'reboot' and suspend/resume work fine.

Looks like the issue I saw is imx6ul-pico board specific.

As Russell pointed out in [2], this is a generic infrastructure problem, and there are other systems where HW might not have a self-IPI. I note that core code already handles that in some cases, e.g. in generic_exec_single where we just disable interrupts and run the work locally rather than sending a self-IPI.

How/where exactly is this self-IPI raised? Can we follow the example of generic_exec_single there?

According to Cortex-A7 MPCore TRM, ch8.3.1, Distributor register summary, GICD_ITARGETSRn:

The register that contains the SGI and PPI interrupts is read-only and the value is implementation defined. For Cortex-A7 configurations with only one processor, these registers are RAZ/WI.

So, the GICD_ITARGETSR[0..7] is read-only, and the value is 0 for cortex-a7 single core platform if the SoC is cortex-a7 mpcore version. So the cupmask from gic_get_cpumask is 0.

At ARM Generic Interrupt Controller Architecture version 2.0, ch4.3.15 Software Generated Interrupt Register, GICD_SGIR, The distributor will process the requested SGI according to register TargetListFilter and CPUTargetList. At current gic code, it takes TargetListFilter as 0b00, and forward the interrupt to cpumask (variable map at gic_raise_softirq) getting from gic_get_cpumask. but cpumask is 0 according to above explanation for cortex-a7 single core platform, so, both TargetListFilter and CPUTargetList are 0, and the distributor does not forward the interrupt to any CPU interface according to gic documentation, then the SGI can't be occurred.

We have found this problem at nxp imx6ul platform, which is a cortex-a7 single core platform, the irq work (triggered by SGI at ARM) can't be occurred which is needed for cpufreq, then the system has failed to boot and reboot [1].

On Mon, 8 Aug 2016 15:49:54 +0800
Peter Chen <[email protected]> wrote:

+Tony

Quoted 23 lines

According to Cortex-A7 MPCore TRM, ch8.3.1, Distributor register summary, GICD_ITARGETSRn:

The register that contains the SGI and PPI interrupts is read-only and the value is implementation defined. For Cortex-A7 configurations with only one processor, these registers are RAZ/WI.

So, the GICD_ITARGETSR[0..7] is read-only, and the value is 0 for cortex-a7 single core platform if the SoC is cortex-a7 mpcore version. So the cupmask from gic_get_cpumask is 0.

At ARM Generic Interrupt Controller Architecture version 2.0, ch4.3.15 Software Generated Interrupt Register, GICD_SGIR, The distributor will process the requested SGI according to register TargetListFilter and CPUTargetList. At current gic code, it takes TargetListFilter as 0b00, and forward the interrupt to cpumask (variable map at gic_raise_softirq) getting from gic_get_cpumask. but cpumask is 0 according to above explanation for cortex-a7 single core platform, so, both TargetListFilter and CPUTargetList are 0, and the distributor does not forward the interrupt to any CPU interface according to gic documentation, then the SGI can't be occurred.

Quoted 4 lines

We have found this problem at nxp imx6ul platform, which is a cortex-a7 single core platform, the irq work (triggered by SGI at ARM) can't be occurred which is needed for cpufreq, then the system has failed to boot and reboot [1].

I'm really not keep on this, as even if we paper over the problem for platforms using a GIC, we still leave behind all the platform that are not capable of self-IPI (which is the vast majority of ARM UP systems).

Can you please provide a backtrace of the failing use case? Have you tried this patch [1], which did solve the issue for OMAP?