[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Peter Chen]

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.

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

Cc: Russell King <linux@armlinux.org.uk>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Jason Liu <jason.hui.liu@nxp.com>
Cc: Anson Huang <anson.huang@nxp.com>
Cc: Frank Li <frank.li@nxp.com>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Ping Bai <ping.bai@nxp.com>

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/irqchip/irq-gic.c | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index c2cab57..625ae6d 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -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);
-- 
1.9.1

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Mark Rutland]

On Mon, Aug 08, 2016 at 03:49:54PM +0800, Peter Chen wrote:
> 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.

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?

Thanks,
Mark.

[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

> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Jason Liu <jason.hui.liu@nxp.com>
> Cc: Anson Huang <anson.huang@nxp.com>
> Cc: Frank Li <frank.li@nxp.com>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Ping Bai <ping.bai@nxp.com>
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/irqchip/irq-gic.c | 39 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index c2cab57..625ae6d 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -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);
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Peter Chen]

 
>
>On Mon, Aug 08, 2016 at 03:49:54PM +0800, Peter Chen wrote:
>> 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.
>
>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.

>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.

> 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.

Peter


>[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
>
>> Cc: Russell King <linux@armlinux.org.uk>
>> Cc: Shawn Guo <shawnguo@kernel.org>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: Jason Liu <jason.hui.liu@nxp.com>
>> Cc: Anson Huang <anson.huang@nxp.com>
>> Cc: Frank Li <frank.li@nxp.com>
>> Cc: Fabio Estevam <fabio.estevam@nxp.com>
>> Cc: Ping Bai <ping.bai@nxp.com>
>>
>> Signed-off-by: Peter Chen <peter.chen@nxp.com>
>> ---
>>  drivers/irqchip/irq-gic.c | 39
>> ++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 36 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index c2cab57..625ae6d 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -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);
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Mark Rutland]

On Mon, Aug 08, 2016 at 12:00:35PM +0000, Peter Chen wrote:
>  
> >
> >On Mon, Aug 08, 2016 at 03:49:54PM +0800, Peter Chen wrote:
> >> 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.
> >
> >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.

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.

> 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.

> >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.
> 
> > 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.

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.

Thanks,
Mark.

> >[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

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Peter Chen]

On Mon, Aug 08, 2016 at 02:07:54PM +0100, Mark Rutland wrote:
> > >>
> > >> 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.
> > 
> > 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.
> 

It doesn't matter. Yes, it is my case.

> > 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.
> 

I think it can work.

> > 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.
> 

How we can detect IPI capabilities based on GIC register?

-- 

Best Regards,
Peter Chen

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Marc Zyngier]

On Mon, 8 Aug 2016 15:49:54 +0800
Peter Chen <peter.chen@nxp.com> wrote:

+Tony

> 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].

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?

Also, if we're going to mandate a self-IPI mechanism for all UP
systems, we do need way to do this in a generic way, as a fallback for
systems that are not SMP aware.

Thanks,

	M.

[1]: https://patchwork.kernel.org/patch/8318231/
-- 
Jazz is not dead. It just smells funny.

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Mark Rutland]

On Mon, Aug 08, 2016 at 09:28:47PM +0800, Peter Chen wrote:
> On Mon, Aug 08, 2016 at 02:07:54PM +0100, Mark Rutland wrote:
> > 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.

[...]

> > 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...

Thanks,
Mark.

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Marc Zyngier]

On Mon, 8 Aug 2016 14:48:42 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Mon, Aug 08, 2016 at 09:28:47PM +0800, Peter Chen wrote:
> > On Mon, Aug 08, 2016 at 02:07:54PM +0100, Mark Rutland wrote:  
> > > 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.  
> 
> [...]
> 
> > > 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...

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

Thanks,

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

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Peter Chen]

On Mon, Aug 08, 2016 at 02:39:35PM +0100, Marc Zyngier wrote:
> On Mon, 8 Aug 2016 15:49:54 +0800
> Peter Chen <peter.chen@nxp.com> wrote:
> 
> +Tony
> 
> > 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].
> 
> 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).
> 

This is a SMP system, not UP system, just cpu number is one.
Current problem is if we support self-IPI for SMP system, when the GIC
in this system doesn't have distribute interrupt capability.

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

This patch is for Feb, 2016, and I use the next-20160722, the related code
is at my kernel (although some other code at context is different).

-- 

Best Regards,
Peter Chen

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Peter Chen]

On Mon, Aug 08, 2016 at 02:59:16PM +0100, Marc Zyngier wrote:
> On Mon, 8 Aug 2016 14:48:42 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > On Mon, Aug 08, 2016 at 09:28:47PM +0800, Peter Chen wrote:
> > > On Mon, Aug 08, 2016 at 02:07:54PM +0100, Mark Rutland wrote:  
> > > > 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.  
> > 
> > [...]
> > 
> > > > 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...
> 
> 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:

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 937c892..276bd94 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -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);
 }

-- 

Best Regards,
Peter Chen

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Marc Zyngier]

On Tue, 9 Aug 2016 11:46:13 +0800
Peter Chen <hzpeterchen@gmail.com> wrote:

> On Mon, Aug 08, 2016 at 02:59:16PM +0100, Marc Zyngier wrote:
> > On Mon, 8 Aug 2016 14:48:42 +0100
> > Mark Rutland <mark.rutland@arm.com> wrote:
> >   
> > > On Mon, Aug 08, 2016 at 09:28:47PM +0800, Peter Chen wrote:  
> > > > On Mon, Aug 08, 2016 at 02:07:54PM +0100, Mark Rutland wrote:    
> > > > > 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.    
> > > 
> > > [...]
> > >   
> > > > > 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...  
> > 
> > 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.

> 
> 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:
> 
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 937c892..276bd94 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -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 <marc.zyngier@arm.com>
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 <marc.zyngier@arm.com>
---
 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(-)

diff --git a/arch/arm/include/asm/irq_work.h b/arch/arm/include/asm/irq_work.h
index 712d03e..025420b 100644
--- a/arch/arm/include/asm/irq_work.h
+++ b/arch/arm/include/asm/irq_work.h
@@ -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 */
diff --git a/arch/arm/include/asm/smp_plat.h b/arch/arm/include/asm/smp_plat.h
index f908071..ffee073 100644
--- a/arch/arm/include/asm/smp_plat.h
+++ b/arch/arm/include/asm/smp_plat.h
@@ -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.
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 8615216..f9d771f 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -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))
 {
-- 
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.

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Peter Chen]

On Tue, Aug 09, 2016 at 06:34:01AM +0100, Marc Zyngier wrote:
> On Tue, 9 Aug 2016 11:46:13 +0800
> Peter Chen <hzpeterchen@gmail.com> wrote:
> 
> > On Mon, Aug 08, 2016 at 02:59:16PM +0100, Marc Zyngier wrote:
> > > On Mon, 8 Aug 2016 14:48:42 +0100
> > > Mark Rutland <mark.rutland@arm.com> wrote:
> > >   
> > > > On Mon, Aug 08, 2016 at 09:28:47PM +0800, Peter Chen wrote:  
> > > > > On Mon, Aug 08, 2016 at 02:07:54PM +0100, Mark Rutland wrote:    
> > > > > > 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.    
> > > > 
> > > > [...]
> > > >   
> > > > > > 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...  
> > > 
> > > 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.
> 

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?

Peter
> > 
> > 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:
> > 
> > diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> > index 937c892..276bd94 100644
> > --- a/arch/arm/kernel/smp.c
> > +++ b/arch/arm/kernel/smp.c
> > @@ -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 <marc.zyngier@arm.com>
> 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 <marc.zyngier@arm.com>
> ---
>  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(-)
> 
> diff --git a/arch/arm/include/asm/irq_work.h b/arch/arm/include/asm/irq_work.h
> index 712d03e..025420b 100644
> --- a/arch/arm/include/asm/irq_work.h
> +++ b/arch/arm/include/asm/irq_work.h
> @@ -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 */
> diff --git a/arch/arm/include/asm/smp_plat.h b/arch/arm/include/asm/smp_plat.h
> index f908071..ffee073 100644
> --- a/arch/arm/include/asm/smp_plat.h
> +++ b/arch/arm/include/asm/smp_plat.h
> @@ -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.
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 8615216..f9d771f 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -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))
>  {
> -- 
> 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.

-- 

Best Regards,
Peter Chen

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Marc Zyngier]

On Tue, 9 Aug 2016 13:57:01 +0800
Peter Chen <hzpeterchen@gmail.com> wrote:

> On Tue, Aug 09, 2016 at 06:34:01AM +0100, Marc Zyngier wrote:
> > On Tue, 9 Aug 2016 11:46:13 +0800
> > Peter Chen <hzpeterchen@gmail.com> wrote:
> >   
> > > On Mon, Aug 08, 2016 at 02:59:16PM +0100, Marc Zyngier wrote:  
> > > > On Mon, 8 Aug 2016 14:48:42 +0100
> > > > Mark Rutland <mark.rutland@arm.com> wrote:
> > > >     
> > > > > On Mon, Aug 08, 2016 at 09:28:47PM +0800, Peter Chen wrote:    
> > > > > > On Mon, Aug 08, 2016 at 02:07:54PM +0100, Mark Rutland wrote:      
> > > > > > > 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.      
> > > > > 
> > > > > [...]
> > > > >     
> > > > > > > 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...    
> > > > 
> > > > 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.
> >   
> 
> Why? Your below patch (I tried too) just fixes NULL pointer issue for

And that's the first issue to solve.

> 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.

> (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.

> 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 <marc.zyngier@arm.com>
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 <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index c2cab57..415aa1e 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -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
-- 
2.8.1

Does it work for you?

Thanks,

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

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Peter Chen]

On Tue, Aug 09, 2016 at 07:59:30AM +0100, Marc Zyngier wrote:
> On Tue, 9 Aug 2016 13:57:01 +0800
> Peter Chen <hzpeterchen@gmail.com> wrote:
> 
> > On Tue, Aug 09, 2016 at 06:34:01AM +0100, Marc Zyngier wrote:
> > > On Tue, 9 Aug 2016 11:46:13 +0800
> > > Peter Chen <hzpeterchen@gmail.com> wrote:
> > >   
> > > > On Mon, Aug 08, 2016 at 02:59:16PM +0100, Marc Zyngier wrote:  
> > > > > On Mon, 8 Aug 2016 14:48:42 +0100
> > > > > Mark Rutland <mark.rutland@arm.com> wrote:
> > > > >     
> > > > > > On Mon, Aug 08, 2016 at 09:28:47PM +0800, Peter Chen wrote:    
> > > > > > > On Mon, Aug 08, 2016 at 02:07:54PM +0100, Mark Rutland wrote:      
> > > > > > > > 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.      
> > > > > > 
> > > > > > [...]
> > > > > >     
> > > > > > > > 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...    
> > > > > 
> > > > > 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.
> > >   
> > 
> > Why? Your below patch (I tried too) just fixes NULL pointer issue for
> 
> And that's the first issue to solve.
> 
> > 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.
> 

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.

> > (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.
> 

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

> > 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:
> 

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.

Peter
> From ad5c001cb1359799831bbb69f8582cb41dda4248 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <marc.zyngier@arm.com>
> 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 <marc.zyngier@arm.com>
> ---
>  drivers/irqchip/irq-gic.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index c2cab57..415aa1e 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -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
> -- 
> 2.8.1
> 
> Does it work for you?
> 
> Thanks,
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny.

-- 

Best Regards,
Peter Chen

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Marc Zyngier]

On 09/08/16 08:18, Peter Chen wrote:
> On Tue, Aug 09, 2016 at 07:59:30AM +0100, Marc Zyngier wrote:
>> On Tue, 9 Aug 2016 13:57:01 +0800
>> Peter Chen <hzpeterchen@gmail.com> wrote:
>>
>>> On Tue, Aug 09, 2016 at 06:34:01AM +0100, Marc Zyngier wrote:
>>>> On Tue, 9 Aug 2016 11:46:13 +0800
>>>> Peter Chen <hzpeterchen@gmail.com> wrote:
>>>>   
>>>>> On Mon, Aug 08, 2016 at 02:59:16PM +0100, Marc Zyngier wrote:  
>>>>>> On Mon, 8 Aug 2016 14:48:42 +0100
>>>>>> Mark Rutland <mark.rutland@arm.com> wrote:
>>>>>>     
>>>>>>> On Mon, Aug 08, 2016 at 09:28:47PM +0800, Peter Chen wrote:    
>>>>>>>> On Mon, Aug 08, 2016 at 02:07:54PM +0100, Mark Rutland wrote:      
>>>>>>>>> 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.      
>>>>>>>
>>>>>>> [...]
>>>>>>>     
>>>>>>>>> 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...    
>>>>>>
>>>>>> 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.
>>>>   
>>>
>>> Why? Your below patch (I tried too) just fixes NULL pointer issue for
>>
>> And that's the first issue to solve.
>>
>>> 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.
>>
> 
> 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.

> 
> 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.
> 
>>> (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.
>>
> 
> Again, it is taken as SMP by kernel, and run SMP instructions, the
> kernel does not do fixup symbol for it.

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).

> 
>>> 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:
>>
> 
> 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 <marc.zyngier@arm.com>
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 <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index c2cab57..390fac5 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -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).

Thanks,

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

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Russell King - ARM Linux]

On Mon, Aug 08, 2016 at 11:50:26AM +0100, Mark Rutland wrote:
> 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?

irq_work_queue_on() calls arch_send_call_function_single_ipi() with
no checks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Peter Chen]

On Tue, Aug 09, 2016 at 09:54:34AM +0100, Marc Zyngier wrote:
> On 09/08/16 08:18, Peter Chen wrote:
> > On Tue, Aug 09, 2016 at 07:59:30AM +0100, Marc Zyngier wrote:
> >> On Tue, 9 Aug 2016 13:57:01 +0800
> >> Peter Chen <hzpeterchen@gmail.com> wrote:
> >>
> >>> On Tue, Aug 09, 2016 at 06:34:01AM +0100, Marc Zyngier wrote:
> >>>> On Tue, 9 Aug 2016 11:46:13 +0800
> >>>> Peter Chen <hzpeterchen@gmail.com> wrote:
> >>>>   
> >>>>> On Mon, Aug 08, 2016 at 02:59:16PM +0100, Marc Zyngier wrote:  
> >>>>>> On Mon, 8 Aug 2016 14:48:42 +0100
> >>>>>> Mark Rutland <mark.rutland@arm.com> wrote:
> >>>>>>     
> >>>>>>> On Mon, Aug 08, 2016 at 09:28:47PM +0800, Peter Chen wrote:    
> >>>>>>>> On Mon, Aug 08, 2016 at 02:07:54PM +0100, Mark Rutland wrote:      
> >>>>>>>>> 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.      
> >>>>>>>
> >>>>>>> [...]
> >>>>>>>     
> >>>>>>>>> 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...    
> >>>>>>
> >>>>>> 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.
> >>>>   
> >>>
> >>> Why? Your below patch (I tried too) just fixes NULL pointer issue for
> >>
> >> And that's the first issue to solve.
> >>
> >>> 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.
> >>
> > 
> > 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).
> 

Thanks, you are right, from hardware level, it is UP system.

> > 
> >>> 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:
> >>
> > 
> > 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:

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>
     c68:	e59f0078 	ldr	r0, [pc, #120]	; ce8 <gic_raise_softirq+0xa4>
     c6c:	ebfffffe 	bl	0 <_raw_spin_lock_irqsave>
     c70:	e3a04000 	mov	r4, #0
     c74:	e59f5070 	ldr	r5, [pc, #112]	; cec <gic_raise_softirq+0xa8>
     c78:	e1a08000 	mov	r8, r0
     c7c:	e3e00000 	mvn	r0, #0
     c80:	ea000001 	b	c8c <gic_raise_softirq+0x48>
     c84:	e5d236ac 	ldrb	r3, [r2, #1708]	; 0x6ac
     c88:	e1844003 	orr	r4, r4, r3
     c8c:	e2802001 	add	r2, r0, #1
     c90:	e3a01004 	mov	r1, #4
     c94:	e1a00006 	mov	r0, r6
     c98:	ebfffffe 	bl	0 <_find_next_bit_le>
     c9c:	e5993000 	ldr	r3, [r9]
     ca0:	e1530000 	cmp	r3, r0
     ca4:	e0852000 	add	r2, r5, r0
     ca8:	cafffff5 	bgt	c84 <gic_raise_softirq+0x40>
     cac:	e3a03000 	mov	r3, #0
     cb0:	ee073fba 	mcr	15, 0, r3, cr7, cr10, {5}
     cb4:	e1874804 	orr	r4, r7, r4, lsl #16
     cb8:	e5953088 	ldr	r3, [r5, #136]	; 0x88
     cbc:	e5834f00 	str	r4, [r3, #3840]	; 0xf00
     cc0:	e59f0020 	ldr	r0, [pc, #32]	; ce8 <gic_raise_softirq+0xa4>
     cc4:	e1a01008 	mov	r1, r8
     cc8:	ebfffffe 	bl	0 <_raw_spin_unlock_irqrestore>
     ccc:	e89dabf0 	ldm	sp, {r4, r5, r6, r7, r8, r9, fp, sp, pc}
     cd0:	e59f3014 	ldr	r3, [pc, #20]	; cec <gic_raise_softirq+0xa8>
     cd4:	e3817402 	orr	r7, r1, #33554432	; 0x2000000
     cd8:	e5933088 	ldr	r3, [r3, #136]	; 0x88
     cdc:	e5837f00 	str	r7, [r3, #3840]	; 0xf00
     ce0:	e89dabf0 	ldm	sp, {r4, r5, r6, r7, r8, r9, fp, sp, pc}
     ce4:	00000000 	.word	0x00000000
     ce8:	00000004 	.word	0x00000004
     cec:	00000000 	.word	0x00000000

- The current code
00000468 <gic_raise_softirq>:
     468:	e1a0c00d 	mov	ip, sp
     46c:	e92ddbf0 	push	{r4, r5, r6, r7, r8, r9, fp, ip, lr, pc}
     470:	e24cb004 	sub	fp, ip, #4
     474:	e1a06000 	mov	r6, r0
     478:	e59f0068 	ldr	r0, [pc, #104]	; 4e8 <gic_raise_softirq+0x80>
     47c:	e1a07001 	mov	r7, r1
     480:	ebfffffe 	bl	0 <_raw_spin_lock_irqsave>
     484:	e3a04000 	mov	r4, #0
     488:	e59f505c 	ldr	r5, [pc, #92]	; 4ec <gic_raise_softirq+0x84>
     48c:	e59f905c 	ldr	r9, [pc, #92]	; 4f0 <gic_raise_softirq+0x88>
     490:	e1a08000 	mov	r8, r0
     494:	e3e00000 	mvn	r0, #0
     498:	ea000001 	b	4a4 <gic_raise_softirq+0x3c>
     49c:	e5d236ac 	ldrb	r3, [r2, #1708]	; 0x6ac
     4a0:	e1844003 	orr	r4, r4, r3
     4a4:	e2802001 	add	r2, r0, #1
     4a8:	e3a01004 	mov	r1, #4
     4ac:	e1a00006 	mov	r0, r6
     4b0:	ebfffffe 	bl	0 <_find_next_bit_le>
     4b4:	e5993000 	ldr	r3, [r9]
     4b8:	e1500003 	cmp	r0, r3
     4bc:	e0852000 	add	r2, r5, r0
     4c0:	bafffff5 	blt	49c <gic_raise_softirq+0x34>
     4c4:	e3a03000 	mov	r3, #0
     4c8:	ee073fba 	mcr	15, 0, r3, cr7, cr10, {5}
     4cc:	e1874804 	orr	r4, r7, r4, lsl #16
     4d0:	e5953088 	ldr	r3, [r5, #136]	; 0x88
     4d4:	e5834f00 	str	r4, [r3, #3840]	; 0xf00
     4d8:	e59f0008 	ldr	r0, [pc, #8]	; 4e8 <gic_raise_softirq+0x80>
     4dc:	e1a01008 	mov	r1, r8
     4e0:	ebfffffe 	bl	0 <_raw_spin_unlock_irqrestore>
     4e4:	e89dabf0 	ldm	sp, {r4, r5, r6, r7, r8, r9, fp, sp, pc}
     4e8:	00000004 	.word	0x00000004

> 
> From 23ba8b645d219e333a10224d74fb5d8d75d67de2 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <marc.zyngier@arm.com>
> 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 <marc.zyngier@arm.com>
> ---
>  drivers/irqchip/irq-gic.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index c2cab57..390fac5 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -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).
> 

Sorry, I don't know the code well.

-- 

Best Regards,
Peter Chen

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Marc Zyngier]

On 09/08/16 10:39, Peter Chen wrote:
> 
> 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.

> 
> - 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.

I bet you won't see anything.

Thanks,

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

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Peter Chen]

On Tue, Aug 09, 2016 at 11:08:49AM +0100, Marc Zyngier wrote:
> On 09/08/16 10:39, Peter Chen wrote:
> > 
> > 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.
> 
> > 
> > - 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.
> 

Sorry, the context was in interrupt at mind when I wrote the last emails.
But in fact, it wasn't. Thanks for help it.

You can add: Tested-by: Peter Chen <peter.chen@nxp.com>

-- 

Best Regards,
Peter Chen

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Fabio Estevam]

Hi Marc,

On Tue, Aug 9, 2016 at 5:54 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:

> From 23ba8b645d219e333a10224d74fb5d8d75d67de2 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <marc.zyngier@arm.com>
> 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 <marc.zyngier@arm.com>

This fixes the halt after running 'reboot' on mx6ul, thanks:

Tested-by: Fabio Estevam <fabio.estevam@nxp.com>

Please also Cc stable when you submit this formally.

Thanks

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Fabio Estevam]

Hi Marc,

On Tue, Aug 9, 2016 at 5:54 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:

> From 23ba8b645d219e333a10224d74fb5d8d75d67de2 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <marc.zyngier@arm.com>
> 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 <marc.zyngier@arm.com>
> ---
>  drivers/irqchip/irq-gic.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index c2cab57..390fac5 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -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);

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

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Marc Zyngier]

On 16/08/16 17:29, Fabio Estevam wrote:
> Hi Marc,
> 
> On Tue, Aug 9, 2016 at 5:54 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> 
>> From 23ba8b645d219e333a10224d74fb5d8d75d67de2 Mon Sep 17 00:00:00 2001
>> From: Marc Zyngier <marc.zyngier@arm.com>
>> 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 <marc.zyngier@arm.com>
>> ---
>>  drivers/irqchip/irq-gic.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index c2cab57..390fac5 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -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);
> 
> 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.

Thanks,

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

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Fabio Estevam]

Hi Marc,

On Tue, Aug 16, 2016 at 1:48 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:

> 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.

Thanks

[PATCH 1/1] irqchip: irq-gic: forward SGI to itself for cortex-a7 single core

[Fabio Estevam]

Hi Marc,

On Tue, Aug 16, 2016 at 2:03 PM, Fabio Estevam <festevam@gmail.com> wrote:

> 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.

Yes, it was a PMIC problem. Will send a fix for it.

Please submit your gic patch, thanks!