[PATCH] gic : check if there are pending interrupts

[PATCH] gic : check if there are pending interrupts

[Daniel Lezcano]

The following patch checks if there are pending interrupts on the gic.

This function is needed for example for the ux500 cpuidle driver.
When the A9 cores and the gic are decoupled from the PRCMU, the idle
routine has to check if an interrupt is pending on the gic before entering
in retention mode.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/common/gic.c               |   37 +++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/hardware/gic.h |    2 +-
 2 files changed, 38 insertions(+), 1 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index aa52699..2528094 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -750,6 +750,43 @@ void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 }
 #endif
 
+/*
+ * gic_pending_irq - checks if there are pending interrupts on the gic
+ *
+ * Disabling an interrupt only disables the forwarding of the
+ * interrupt to any CPU interface. It does not prevent the interrupt
+ * from changing state, for example becoming pending, or active and
+ * pending if it is already active. For this reason, we have to check
+ * the interrupt is pending *and* is enabled.
+ *
+ * Returns true if there are pending and enabled interrupts, false
+ * otherwise.
+ */
+bool gic_pending_irq(unsigned int gic_nr)
+{
+	u32 pr; /* Pending register */
+	u32 er; /* Enable register */
+	void __iomem *dist_base;
+	int gic_irqs;
+	int i;
+
+	BUG_ON(gic_nr >= MAX_GIC_NR);
+
+	gic_irqs = gic_data[gic_nr].gic_irqs;
+	dist_base = gic_data_dist_base(&gic_data[gic_nr]);
+
+	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 32); i++) {
+
+		pr = readl_relaxed(dist_base + GIC_DIST_PENDING_SET + i * 4);
+		er = readl_relaxed(dist_base + GIC_DIST_ENABLE_SET + i * 4);
+
+		if (pr & er)
+			return true;
+	}
+
+	return false;
+}
+
 #ifdef CONFIG_OF
 static int gic_cnt __initdata = 0;
 
diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h
index 4b1ce6c..d198ac0 100644
--- a/arch/arm/include/asm/hardware/gic.h
+++ b/arch/arm/include/asm/hardware/gic.h
@@ -45,7 +45,7 @@ void gic_secondary_init(unsigned int);
 void gic_handle_irq(struct pt_regs *regs);
 void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
 void gic_raise_softirq(const struct cpumask *mask, unsigned int irq);
-
+bool gic_pending_irq(unsigned int gic_nr);
 static inline void gic_init(unsigned int nr, int start,
 			    void __iomem *dist , void __iomem *cpu)
 {
-- 
1.7.5.4

[PATCH] gic : check if there are pending interrupts

[Rob Herring]

On 02/24/2012 07:45 AM, Daniel Lezcano wrote:
> The following patch checks if there are pending interrupts on the gic.
> 
> This function is needed for example for the ux500 cpuidle driver.
> When the A9 cores and the gic are decoupled from the PRCMU, the idle
> routine has to check if an interrupt is pending on the gic before entering
> in retention mode.

That sounds racy. Soon as you check an interrupt can assert.

> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/common/gic.c               |   37 +++++++++++++++++++++++++++++++++++
>  arch/arm/include/asm/hardware/gic.h |    2 +-
>  2 files changed, 38 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index aa52699..2528094 100644
> --- a/arch/arm/common/gic.c
> +++ b/arch/arm/common/gic.c
> @@ -750,6 +750,43 @@ void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>  }
>  #endif
>  
> +/*
> + * gic_pending_irq - checks if there are pending interrupts on the gic
> + *
> + * Disabling an interrupt only disables the forwarding of the
> + * interrupt to any CPU interface. It does not prevent the interrupt
> + * from changing state, for example becoming pending, or active and
> + * pending if it is already active. For this reason, we have to check
> + * the interrupt is pending *and* is enabled.
> + *
> + * Returns true if there are pending and enabled interrupts, false
> + * otherwise.
> + */
> +bool gic_pending_irq(unsigned int gic_nr)

Seems like this should be solved with a generic interface and not gic
specific.

Would we ever need this for a secondary gic (gic_nr != 0)?

> +{
> +	u32 pr; /* Pending register */
> +	u32 er; /* Enable register */
> +	void __iomem *dist_base;
> +	int gic_irqs;
> +	int i;
> +
> +	BUG_ON(gic_nr >= MAX_GIC_NR);
> +
> +	gic_irqs = gic_data[gic_nr].gic_irqs;
> +	dist_base = gic_data_dist_base(&gic_data[gic_nr]);
> +
> +	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 32); i++) {

Wouldn't you want to skip PPIs if the CPU interface is disabled?

> +
> +		pr = readl_relaxed(dist_base + GIC_DIST_PENDING_SET + i * 4);
> +		er = readl_relaxed(dist_base + GIC_DIST_ENABLE_SET + i * 4);
> +

What if an interrupt is pending, but routed to a core which is not
getting put into low power state?

Rob

> +		if (pr & er)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  #ifdef CONFIG_OF
>  static int gic_cnt __initdata = 0;
>  
> diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h
> index 4b1ce6c..d198ac0 100644
> --- a/arch/arm/include/asm/hardware/gic.h
> +++ b/arch/arm/include/asm/hardware/gic.h
> @@ -45,7 +45,7 @@ void gic_secondary_init(unsigned int);
>  void gic_handle_irq(struct pt_regs *regs);
>  void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
>  void gic_raise_softirq(const struct cpumask *mask, unsigned int irq);
> -
> +bool gic_pending_irq(unsigned int gic_nr);
>  static inline void gic_init(unsigned int nr, int start,
>  			    void __iomem *dist , void __iomem *cpu)
>  {

[PATCH] gic : check if there are pending interrupts

[Russell King - ARM Linux]

On Fri, Feb 24, 2012 at 02:45:48PM +0100, Daniel Lezcano wrote:
> The following patch checks if there are pending interrupts on the gic.
> 
> This function is needed for example for the ux500 cpuidle driver.
> When the A9 cores and the gic are decoupled from the PRCMU, the idle
> routine has to check if an interrupt is pending on the gic before entering
> in retention mode.

So what happens if an interrupt becomes pending after you've checked it
but before you've gone into retention mode?  This basically sounds like
one big racy idea to me.

Hardware which allows you to go into retention mode with interrupts
pending which should prevent it (or at least cause it to re-awake) just
sounds like a massive fail to me.

[PATCH] gic : check if there are pending interrupts

[Daniel Lezcano]

On 02/24/2012 07:27 PM, Russell King - ARM Linux wrote:
> On Fri, Feb 24, 2012 at 02:45:48PM +0100, Daniel Lezcano wrote:
>> The following patch checks if there are pending interrupts on the gic.
>>
>> This function is needed for example for the ux500 cpuidle driver.
>> When the A9 cores and the gic are decoupled from the PRCMU, the idle
>> routine has to check if an interrupt is pending on the gic before entering
>> in retention mode.
>
> So what happens if an interrupt becomes pending after you've checked it
> but before you've gone into retention mode?  This basically sounds like
> one big racy idea to me.

The retention mode for the ux500 SoC is the following:

1 - the gic is decoupled : no interrupts will occur and wake up the 
second core if it is in WFI

2 - the gic settings are copied to the PRCMU which is in charge to wake 
up the cores and recouple the gic. We have to check here if an 
interrupts occurred between 1 and 2 in order to abort the retention mode 
and recouple the gic.

3 - we go to retention mode where the PRCMU waits for both cores to be 
in WFI, then if an interrupt is pending (the interrupt occurred between 
2 -3), or occurs, it recouples the gic and wakes the cores.

Here is the implementation to illustrate this.

static inline int ux500_enter_arm_retention(struct cpuidle_device *dev,
					    struct cpuidle_driver *drv,
						int index)
{
	int this_cpu = smp_processor_id();
	bool recouple = false;

	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &this_cpu);

	if (atomic_inc_return(&master) == num_online_cpus()) {

		/* With this lock, we prevent the other cpu to exit and enter
		 * this function again and become the master */
		if (!spin_trylock(&master_lock))
			goto wfi;

		WARN_ONCE(prcmu_gic_decouple(), "Failed to decouple gic from prcmu");

		/* If an abort occurs, we will have to recouple the gic
		 * manually */
		recouple = true;

		/* At this state, as the gic is decoupled, if the other
		 * cpu is in WFI, we have the guarantee it won't be wake
		 * up, so we can safely go to retention */
		if (!ux500_pm_other_cpu_wfi(this_cpu))
			goto out;

		/* The prcmu will be in charge of watching the interrupts
		 * and wake up the cpus */
		ux500_pm_prcmu_copy_gic_settings();

		/* Check in the meantime an interrupt did
		 * not occur on the gic ... */
		if (gic_pending_irq(0))
			goto out;

		/* ... and the prcmu */
		if (ux500_pm_prcmu_pending_interrupt())
			goto out;

		/* Go to the retention state, the prcmu will wait for the
		 * cpu to go WFI and this is what happens after exiting this
		 * 'master' critical section */
		if (prcmu_set_power_state(PRCMU_AP_IDLE, true, true))
			goto out;

		/* When we switch to retention, the prcmu is in charge
		 * of recoupling the gic automatically */
		recouple = false;

		spin_unlock(&master_lock);
	}
wfi:
	cpu_do_idle();
out:
	atomic_dec(&master);

	if (recouple) {
		WARN_ONCE(prcmu_gic_recouple(), "Failed to recouple gic to prcmu");
		spin_unlock(&master_lock);
	}

	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &this_cpu);

	return index;
}


The function to check if there are pending interrupts on the gic is 
needed for the cpuidle driver, as well as a function to copy the gic 
settings to the PRCMU. Maybe this is too SoC specific and the function 
could moved to the ux500 with some information exported from gic.c, no ?


> Hardware which allows you to go into retention mode with interrupts
> pending which should prevent it (or at least cause it to re-awake) just
> sounds like a massive fail to me.

Well, I don't have an opinion on this :)

Thanks
   -- Daniel

-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH] gic : check if there are pending interrupts

[Daniel Lezcano]

On 02/24/2012 05:49 PM, Rob Herring wrote:
> On 02/24/2012 07:45 AM, Daniel Lezcano wrote:
>> The following patch checks if there are pending interrupts on the gic.
>>
>> This function is needed for example for the ux500 cpuidle driver.
>> When the A9 cores and the gic are decoupled from the PRCMU, the idle
>> routine has to check if an interrupt is pending on the gic before entering
>> in retention mode.
>
> That sounds racy. Soon as you check an interrupt can assert.

Yes it sounds racy but not for the ux500 where the gic can be decoupled 
and the irq are catched by the PRCMU to wake up the core A9.

>> Signed-off-by: Daniel Lezcano<daniel.lezcano@linaro.org>
>> ---
>>   arch/arm/common/gic.c               |   37 +++++++++++++++++++++++++++++++++++
>>   arch/arm/include/asm/hardware/gic.h |    2 +-
>>   2 files changed, 38 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>> index aa52699..2528094 100644
>> --- a/arch/arm/common/gic.c
>> +++ b/arch/arm/common/gic.c
>> @@ -750,6 +750,43 @@ void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>>   }
>>   #endif
>>
>> +/*
>> + * gic_pending_irq - checks if there are pending interrupts on the gic
>> + *
>> + * Disabling an interrupt only disables the forwarding of the
>> + * interrupt to any CPU interface. It does not prevent the interrupt
>> + * from changing state, for example becoming pending, or active and
>> + * pending if it is already active. For this reason, we have to check
>> + * the interrupt is pending *and* is enabled.
>> + *
>> + * Returns true if there are pending and enabled interrupts, false
>> + * otherwise.
>> + */
>> +bool gic_pending_irq(unsigned int gic_nr)
>
> Seems like this should be solved with a generic interface and not gic
> specific.


> Would we ever need this for a secondary gic (gic_nr != 0)?

I don't think so for this specific routine which will be used by the 
ux500 for the moment.

>> +{
>> +	u32 pr; /* Pending register */
>> +	u32 er; /* Enable register */
>> +	void __iomem *dist_base;
>> +	int gic_irqs;
>> +	int i;
>> +
>> +	BUG_ON(gic_nr>= MAX_GIC_NR);
>> +
>> +	gic_irqs = gic_data[gic_nr].gic_irqs;
>> +	dist_base = gic_data_dist_base(&gic_data[gic_nr]);
>> +
>> +	for (i = 0; i<  DIV_ROUND_UP(gic_irqs, 32); i++) {
>
> Wouldn't you want to skip PPIs if the CPU interface is disabled?
>
>> +
>> +		pr = readl_relaxed(dist_base + GIC_DIST_PENDING_SET + i * 4);
>> +		er = readl_relaxed(dist_base + GIC_DIST_ENABLE_SET + i * 4);
>> +
>
> What if an interrupt is pending, but routed to a core which is not
> getting put into low power state?

Hmm, right. That does not happen with the ux500 because the gic is 
decoupled from the cores. Checking if some irq are pending on the gic 
makes sense only on this case which is very specific to the ux500 SoC.
I am wondering if that would not make sense to move this routine to the 
mfd's prcmu.c file, no ?

   -- Daniel

-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog