[PATCH] ARM: imx6q: cpuidle: fix bug that CPU may not wake up

[PATCH] ARM: imx6q: cpuidle: fix bug that CPU may not wake up

[Kohji Okuno]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

Hi,
I found the bug in cpuidle-imx6q.c. Could you check my patch?

 In the current cpuidle implementation for i.MX6q, the CPU that sets
"WAIT_UNCLOCKED" and the CPU that returns to "WAIT_CLOCKED" are always
the same. While the CPU that sets "WAIT_UNCLOCKED" is in IDLE state of
"WAIT", if the other CPU wakes up and enters IDLE state of "WFI"
istead of "WAIT", this CPU can not wake up at expired time.
 Because, in the case of "WFI", the CPU must be waked up by the local
timer interrupt. But, while "WAIT_UNCLOCKED" is set, the local timer
is stopped, when all CPUs execute "wfi" instruction. As a result, the
local timer interrupt is not fired.
 In this situation, this CPU will wake up by IRQ different from local
timer. (e.g. broacast tiemr)

So, this fix changes CPU to return to "WAIT_CLOCKED".

Best regards,
 Kohji Okuno

[-- Attachment #2: 0001-ARM-imx6q-cpuidle-fix-bug-that-CPU-may-not-wake-up-a.patch --]
[-- Type: text/x-patch, Size: 2265 bytes --]

From 2cfeb5f68c2dd0de4854dbac091bf330e25f00d1 Mon Sep 17 00:00:00 2001
From: Kohji Okuno <okuno.kohji@jp.panasonic.com>
Date: Fri, 22 Feb 2019 17:45:26 +0900
Subject: [PATCH] ARM: imx6q: cpuidle: fix bug that CPU may not wake up at
 expected time

 In the current cpuidle implementation for i.MX6q, the CPU that sets
"WAIT_UNCLOCKED" and the CPU that returns to "WAIT_CLOCKED" are always
the same. While the CPU that sets "WAIT_UNCLOCKED" is in IDLE state of
"WAIT", if the other CPU wakes up and enters IDLE state of "WFI"
istead of "WAIT", this CPU can not wake up at expired time.
 Because, in the case of "WFI", the CPU must be waked up by the local
timer interrupt. But, while "WAIT_UNCLOCKED" is set, the local timer
is stopped, when all CPUs execute "wfi" instruction. As a result, the
local timer interrupt is not fired.
 In this situation, this CPU will wake up by IRQ different from local
timer. (e.g. broacast tiemr)

So, this fix changes CPU to return to "WAIT_CLOCKED".
---
 arch/arm/mach-imx/cpuidle-imx6q.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c
index bfeb25aaf9a2..22d982d451d1 100644
--- a/arch/arm/mach-imx/cpuidle-imx6q.c
+++ b/arch/arm/mach-imx/cpuidle-imx6q.c
@@ -16,30 +16,25 @@
 #include "cpuidle.h"
 #include "hardware.h"
 
-static atomic_t master = ATOMIC_INIT(0);
+static int master = 0;
 static DEFINE_SPINLOCK(master_lock);
 
 static int imx6q_enter_wait(struct cpuidle_device *dev,
 			    struct cpuidle_driver *drv, int index)
 {
-	if (atomic_inc_return(&master) == num_online_cpus()) {
-		/*
-		 * With this lock, we prevent other cpu to exit and enter
-		 * this function again and become the master.
-		 */
-		if (!spin_trylock(&master_lock))
-			goto idle;
+	spin_lock(&master_lock);
+	if (++master == num_online_cpus()) {
 		imx6_set_lpm(WAIT_UNCLOCKED);
-		cpu_do_idle();
-		imx6_set_lpm(WAIT_CLOCKED);
-		spin_unlock(&master_lock);
-		goto done;
 	}
+	spin_unlock(&master_lock);
 
-idle:
 	cpu_do_idle();
-done:
-	atomic_dec(&master);
+
+	spin_lock(&master_lock);
+	if (master-- == num_online_cpus()) {
+		imx6_set_lpm(WAIT_CLOCKED);
+	}
+	spin_unlock(&master_lock);
 
 	return index;
 }
-- 
2.17.1


[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH] ARM: imx6q: cpuidle: fix bug that CPU may not wake up

[Lucas Stach]

Hi,

Am Freitag, den 22.02.2019, 17:49 +0900 schrieb Kohji Okuno:
> Hi,
> I found the bug in cpuidle-imx6q.c. Could you check my patch?
> 
>  In the current cpuidle implementation for i.MX6q, the CPU that sets
> "WAIT_UNCLOCKED" and the CPU that returns to "WAIT_CLOCKED" are
> always
> the same. While the CPU that sets "WAIT_UNCLOCKED" is in IDLE state
> of
> "WAIT", if the other CPU wakes up and enters IDLE state of "WFI"
> istead of "WAIT", this CPU can not wake up at expired time.
>  Because, in the case of "WFI", the CPU must be waked up by the local
> timer interrupt. But, while "WAIT_UNCLOCKED" is set, the local timer
> is stopped, when all CPUs execute "wfi" instruction. As a result, the
> local timer interrupt is not fired.
>  In this situation, this CPU will wake up by IRQ different from local
> timer. (e.g. broacast tiemr)
> 
> So, this fix changes CPU to return to "WAIT_CLOCKED".

Please send patches inline, instead of attachments. That makes review a
lot easier.

I agree that the patch does the right thing, while simplifying the code
at the same time, which is always a good sign. The only (rather minor)
issue that I could spot is that the "master" variable name isn't
matching what it does anymore, as there is no master CPU anymore. It is
more like the last man standing turning the clock off, while the first
one to wake up will switch it back on. But then I also fail to come up
with a better naming.

Regards,
Lucas


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

Re: [PATCH] ARM: imx6q: cpuidle: fix bug that CPU may not wake up

[Fabio Estevam]

Hi Kohji,

On Fri, Feb 22, 2019 at 6:14 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi,
>
> Am Freitag, den 22.02.2019, 17:49 +0900 schrieb Kohji Okuno:
> > Hi,
> > I found the bug in cpuidle-imx6q.c. Could you check my patch?
> >
> >  In the current cpuidle implementation for i.MX6q, the CPU that sets
> > "WAIT_UNCLOCKED" and the CPU that returns to "WAIT_CLOCKED" are
> > always
> > the same. While the CPU that sets "WAIT_UNCLOCKED" is in IDLE state
> > of
> > "WAIT", if the other CPU wakes up and enters IDLE state of "WFI"
> > istead of "WAIT", this CPU can not wake up at expired time.
> >  Because, in the case of "WFI", the CPU must be waked up by the local
> > timer interrupt. But, while "WAIT_UNCLOCKED" is set, the local timer
> > is stopped, when all CPUs execute "wfi" instruction. As a result, the
> > local timer interrupt is not fired.
> >  In this situation, this CPU will wake up by IRQ different from local
> > timer. (e.g. broacast tiemr)
> >
> > So, this fix changes CPU to return to "WAIT_CLOCKED".
>
> Please send patches inline, instead of attachments. That makes review a
> lot easier.

Yes, please re-send it with "git send-email" and also include your
Signed-off-by tag.

It looks like a good fix.

Thanks

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

[PATCH v2] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time

[Kohji Okuno]

 In the current cpuidle implementation for i.MX6q, the CPU that sets
'WAIT_UNCLOCKED' and the CPU that returns to 'WAIT_CLOCKED' are always
the same. While the CPU that sets 'WAIT_UNCLOCKED' is in IDLE state of
"WAIT", if the other CPU wakes up and enters IDLE state of "WFI"
istead of "WAIT", this CPU can not wake up at expired time.
 Because, in the case of "WFI", the CPU must be waked up by the local
timer interrupt. But, while 'WAIT_UNCLOCKED' is set, the local timer
is stopped, when all CPUs execute "wfi" instruction. As a result, the
local timer interrupt is not fired.
 In this situation, this CPU will wake up by IRQ different from local
timer. (e.g. broacast tiemr)

 So, this fix changes CPU to return to 'WAIT_CLOCKED'.

Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>
---

Changes for v2:
 - change "master" variable name.


 arch/arm/mach-imx/cpuidle-imx6q.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c
index bfeb25aaf9a2..9d005b589498 100644
--- a/arch/arm/mach-imx/cpuidle-imx6q.c
+++ b/arch/arm/mach-imx/cpuidle-imx6q.c
@@ -16,30 +16,25 @@
 #include "cpuidle.h"
 #include "hardware.h"
 
-static atomic_t master = ATOMIC_INIT(0);
-static DEFINE_SPINLOCK(master_lock);
+static int num_idle_cpus = 0;
+static DEFINE_SPINLOCK(cpuidle_lock);
 
 static int imx6q_enter_wait(struct cpuidle_device *dev,
 			    struct cpuidle_driver *drv, int index)
 {
-	if (atomic_inc_return(&master) == num_online_cpus()) {
-		/*
-		 * With this lock, we prevent other cpu to exit and enter
-		 * this function again and become the master.
-		 */
-		if (!spin_trylock(&master_lock))
-			goto idle;
+	spin_lock(&cpuidle_lock);
+	if (++num_idle_cpus == num_online_cpus()) {
 		imx6_set_lpm(WAIT_UNCLOCKED);
-		cpu_do_idle();
-		imx6_set_lpm(WAIT_CLOCKED);
-		spin_unlock(&master_lock);
-		goto done;
 	}
+	spin_unlock(&cpuidle_lock);
 
-idle:
 	cpu_do_idle();
-done:
-	atomic_dec(&master);
+
+	spin_lock(&cpuidle_lock);
+	if (num_idle_cpus-- == num_online_cpus()) {
+		imx6_set_lpm(WAIT_CLOCKED);
+	}
+	spin_unlock(&cpuidle_lock);
 
 	return index;
 }
-- 
2.17.1


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

Re: [PATCH v2] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time

[Fabio Estevam]

Hi Kohji,

On Mon, Feb 25, 2019 at 11:08 PM Kohji Okuno
<okuno.kohji@jp.panasonic.com> wrote:

> +       if (++num_idle_cpus == num_online_cpus()) {
>                 imx6_set_lpm(WAIT_UNCLOCKED);

You should remove the braces for a single statement inside the if block.

> -               cpu_do_idle();
> -               imx6_set_lpm(WAIT_CLOCKED);
> -               spin_unlock(&master_lock);
> -               goto done;
>         }
> +       spin_unlock(&cpuidle_lock);
>
> -idle:
>         cpu_do_idle();
> -done:
> -       atomic_dec(&master);
> +
> +       spin_lock(&cpuidle_lock);
> +       if (num_idle_cpus-- == num_online_cpus()) {
> +               imx6_set_lpm(WAIT_CLOCKED);
> +       }

Same here.

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

Re: [PATCH v2] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time

[Kohji Okuno]

Hi Fabio,

Than you for your comment.
Just to be sure, let me check.

Should we change as follows?

-      if (++num_idle_cpus == num_online_cpus()) {
+      num_idle_cpus++
+      if (num_idle_cpus == num_online_cpus()) {

Fabio Estevam <festevam@gmail.com> wrote:
> Hi Kohji,
> 
> On Mon, Feb 25, 2019 at 11:08 PM Kohji Okuno
> <okuno.kohji@jp.panasonic.com> wrote:
> 
>> +       if (++num_idle_cpus == num_online_cpus()) {
>>                 imx6_set_lpm(WAIT_UNCLOCKED);
> 
> You should remove the braces for a single statement inside the if block.
> 
>> -               cpu_do_idle();
>> -               imx6_set_lpm(WAIT_CLOCKED);
>> -               spin_unlock(&master_lock);
>> -               goto done;
>>         }
>> +       spin_unlock(&cpuidle_lock);
>>
>> -idle:
>>         cpu_do_idle();
>> -done:
>> -       atomic_dec(&master);
>> +
>> +       spin_lock(&cpuidle_lock);
>> +       if (num_idle_cpus-- == num_online_cpus()) {
>> +               imx6_set_lpm(WAIT_CLOCKED);
>> +       }
> 
> Same here.

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

Re: [PATCH v2] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time

[Fabio Estevam]

Hi Kohji,

On Mon, Feb 25, 2019 at 11:19 PM Kohji Okuno
<okuno.kohji@jp.panasonic.com> wrote:
>
> Hi Fabio,
>
> Than you for your comment.
> Just to be sure, let me check.
>
> Should we change as follows?
>
> -      if (++num_idle_cpus == num_online_cpus()) {
> +      num_idle_cpus++
> +      if (num_idle_cpus == num_online_cpus()) {

Instead of doing:

       if (num_idle_cpus-- == num_online_cpus()) {
               imx6_set_lpm(WAIT_CLOCKED);
       }

You should do:

       if (num_idle_cpus-- == num_online_cpus())
               imx6_set_lpm(WAIT_CLOCKED);

No need for adding { for an if block if only one statement is used.

Thanks

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

Re: [PATCH v2] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time

[Kohji Okuno]

Hi Fabio,

Was your meaning as follows?

-       if (++num_idle_cpus == num_online_cpus()) {
+       if (++num_idle_cpus == num_online_cpus())
                imx6_set_lpm(WAIT_UNCLOCKED);
-       }

Best regards,
 Kohji Okuno

Kohji Okuno <okuno.kohji@jp.panasonic.com> wrote:
> Hi Fabio,
> 
> Than you for your comment.
> Just to be sure, let me check.
> 
> Should we change as follows?
> 
> -      if (++num_idle_cpus == num_online_cpus()) {
> +      num_idle_cpus++
> +      if (num_idle_cpus == num_online_cpus()) {
> 
> Fabio Estevam <festevam@gmail.com> wrote:
>> Hi Kohji,
>> 
>> On Mon, Feb 25, 2019 at 11:08 PM Kohji Okuno
>> <okuno.kohji@jp.panasonic.com> wrote:
>> 
>>> +       if (++num_idle_cpus == num_online_cpus()) {
>>>                 imx6_set_lpm(WAIT_UNCLOCKED);
>> 
>> You should remove the braces for a single statement inside the if block.
>> 
>>> -               cpu_do_idle();
>>> -               imx6_set_lpm(WAIT_CLOCKED);
>>> -               spin_unlock(&master_lock);
>>> -               goto done;
>>>         }
>>> +       spin_unlock(&cpuidle_lock);
>>>
>>> -idle:
>>>         cpu_do_idle();
>>> -done:
>>> -       atomic_dec(&master);
>>> +
>>> +       spin_lock(&cpuidle_lock);
>>> +       if (num_idle_cpus-- == num_online_cpus()) {
>>> +               imx6_set_lpm(WAIT_CLOCKED);
>>> +       }
>> 
>> Same here.

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

[PATCH v3] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time

[Kohji Okuno]

 In the current cpuidle implementation for i.MX6q, the CPU that sets
'WAIT_UNCLOCKED' and the CPU that returns to 'WAIT_CLOCKED' are always
the same. While the CPU that sets 'WAIT_UNCLOCKED' is in IDLE state of
"WAIT", if the other CPU wakes up and enters IDLE state of "WFI"
istead of "WAIT", this CPU can not wake up at expired time.
 Because, in the case of "WFI", the CPU must be waked up by the local
timer interrupt. But, while 'WAIT_UNCLOCKED' is set, the local timer
is stopped, when all CPUs execute "wfi" instruction. As a result, the
local timer interrupt is not fired.
 In this situation, this CPU will wake up by IRQ different from local
timer. (e.g. broacast tiemr)

So, this fix changes CPU to return to 'WAIT_CLOCKED'.

Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>
---

Changes for v3:
 - remove the braces for a single statement inside the if block.

Changes for v2:
 - change "master" variable name.

 arch/arm/mach-imx/cpuidle-imx6q.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c
index bfeb25aaf9a2..326e870d7123 100644
--- a/arch/arm/mach-imx/cpuidle-imx6q.c
+++ b/arch/arm/mach-imx/cpuidle-imx6q.c
@@ -16,30 +16,23 @@
 #include "cpuidle.h"
 #include "hardware.h"
 
-static atomic_t master = ATOMIC_INIT(0);
-static DEFINE_SPINLOCK(master_lock);
+static int num_idle_cpus = 0;
+static DEFINE_SPINLOCK(cpuidle_lock);
 
 static int imx6q_enter_wait(struct cpuidle_device *dev,
 			    struct cpuidle_driver *drv, int index)
 {
-	if (atomic_inc_return(&master) == num_online_cpus()) {
-		/*
-		 * With this lock, we prevent other cpu to exit and enter
-		 * this function again and become the master.
-		 */
-		if (!spin_trylock(&master_lock))
-			goto idle;
+	spin_lock(&cpuidle_lock);
+	if (++num_idle_cpus == num_online_cpus())
 		imx6_set_lpm(WAIT_UNCLOCKED);
-		cpu_do_idle();
-		imx6_set_lpm(WAIT_CLOCKED);
-		spin_unlock(&master_lock);
-		goto done;
-	}
+	spin_unlock(&cpuidle_lock);
 
-idle:
 	cpu_do_idle();
-done:
-	atomic_dec(&master);
+
+	spin_lock(&cpuidle_lock);
+	if (num_idle_cpus-- == num_online_cpus())
+		imx6_set_lpm(WAIT_CLOCKED);
+	spin_unlock(&cpuidle_lock);
 
 	return index;
 }
-- 
2.17.1


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

Re: [PATCH v3] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time

[Shawn Guo]

Hi Kohji,

On Tue, Feb 26, 2019 at 11:34:13AM +0900, Kohji Okuno wrote:
>  In the current cpuidle implementation for i.MX6q, the CPU that sets
> 'WAIT_UNCLOCKED' and the CPU that returns to 'WAIT_CLOCKED' are always
> the same. While the CPU that sets 'WAIT_UNCLOCKED' is in IDLE state of
> "WAIT", if the other CPU wakes up and enters IDLE state of "WFI"
> istead of "WAIT", this CPU can not wake up at expired time.
>  Because, in the case of "WFI", the CPU must be waked up by the local
> timer interrupt. But, while 'WAIT_UNCLOCKED' is set, the local timer
> is stopped, when all CPUs execute "wfi" instruction. As a result, the
> local timer interrupt is not fired.
>  In this situation, this CPU will wake up by IRQ different from local
> timer. (e.g. broacast tiemr)
> 
> So, this fix changes CPU to return to 'WAIT_CLOCKED'.

Just out of curiosity, how did you observe the bug?  I'm trying to
understand the impact of the bug.

Shawn

> 
> Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>
> ---
> 
> Changes for v3:
>  - remove the braces for a single statement inside the if block.
> 
> Changes for v2:
>  - change "master" variable name.
> 
>  arch/arm/mach-imx/cpuidle-imx6q.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c
> index bfeb25aaf9a2..326e870d7123 100644
> --- a/arch/arm/mach-imx/cpuidle-imx6q.c
> +++ b/arch/arm/mach-imx/cpuidle-imx6q.c
> @@ -16,30 +16,23 @@
>  #include "cpuidle.h"
>  #include "hardware.h"
>  
> -static atomic_t master = ATOMIC_INIT(0);
> -static DEFINE_SPINLOCK(master_lock);
> +static int num_idle_cpus = 0;
> +static DEFINE_SPINLOCK(cpuidle_lock);
>  
>  static int imx6q_enter_wait(struct cpuidle_device *dev,
>  			    struct cpuidle_driver *drv, int index)
>  {
> -	if (atomic_inc_return(&master) == num_online_cpus()) {
> -		/*
> -		 * With this lock, we prevent other cpu to exit and enter
> -		 * this function again and become the master.
> -		 */
> -		if (!spin_trylock(&master_lock))
> -			goto idle;
> +	spin_lock(&cpuidle_lock);
> +	if (++num_idle_cpus == num_online_cpus())
>  		imx6_set_lpm(WAIT_UNCLOCKED);
> -		cpu_do_idle();
> -		imx6_set_lpm(WAIT_CLOCKED);
> -		spin_unlock(&master_lock);
> -		goto done;
> -	}
> +	spin_unlock(&cpuidle_lock);
>  
> -idle:
>  	cpu_do_idle();
> -done:
> -	atomic_dec(&master);
> +
> +	spin_lock(&cpuidle_lock);
> +	if (num_idle_cpus-- == num_online_cpus())
> +		imx6_set_lpm(WAIT_CLOCKED);
> +	spin_unlock(&cpuidle_lock);
>  
>  	return index;
>  }
> -- 
> 2.17.1
> 

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

Re: [PATCH v3] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time

[Kohji Okuno]

Hi Shawn,

I found this bug by delaying periodic communication. For example, it
is the source codes as below.

	while (1) {
		sleep(2);
		write(fd, "Hello", strlen("Hello"));
	}

Of course, it dows not occur frequently. However, in the worst case,
it stopped for more than 1 minute.

If there are few running processes and few hrtimer events, this issue
will occur.

For example:
(*) time is abstracting.
    WFI idle : the CPU will be waked up by local timer.
    WAIT idle: the CPU will be waked up by broad cast(bc) timer.
    
[time]
	  CPU#0				  CPU#1
----------------------------------------------------------------------
	enter "WAIT" idle
	[next event=10]
					enter "WAIT" idle
					[next event=60]	
			all cpus are IDLE = > CPU#1 sets WAIT_UNCLOCKED

10:	bc timer fires
	exit idle
	
	enter "WFI" idle
	[next event=12]
	
12:     ** local timer is not fired **
	CPU#0 will not be waked up
	until the next bc timer interrupt.

			>> all processes stop <<

60:	bc timer fires
	exit idle			exit idle
					CPU#1 sets WAIT_CLOCKED
	local timer fires

If CPU#0 sets WAIT_CLOCKED at time=10, this issue will not occur.

Does that make any sense?

Best regards,
 Kohji Okuno


Shawn Guo <shawnguo@kernel.org> wrote:
> Hi Kohji,
> 
> On Tue, Feb 26, 2019 at 11:34:13AM +0900, Kohji Okuno wrote:
>>  In the current cpuidle implementation for i.MX6q, the CPU that sets
>> 'WAIT_UNCLOCKED' and the CPU that returns to 'WAIT_CLOCKED' are always
>> the same. While the CPU that sets 'WAIT_UNCLOCKED' is in IDLE state of
>> "WAIT", if the other CPU wakes up and enters IDLE state of "WFI"
>> istead of "WAIT", this CPU can not wake up at expired time.
>>  Because, in the case of "WFI", the CPU must be waked up by the local
>> timer interrupt. But, while 'WAIT_UNCLOCKED' is set, the local timer
>> is stopped, when all CPUs execute "wfi" instruction. As a result, the
>> local timer interrupt is not fired.
>>  In this situation, this CPU will wake up by IRQ different from local
>> timer. (e.g. broacast tiemr)
>> 
>> So, this fix changes CPU to return to 'WAIT_CLOCKED'.
> 
> Just out of curiosity, how did you observe the bug?  I'm trying to
> understand the impact of the bug.
> 
> Shawn
> 
>> 
>> Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>
>> ---
>> 
>> Changes for v3:
>>  - remove the braces for a single statement inside the if block.
>> 
>> Changes for v2:
>>  - change "master" variable name.
>> 
>>  arch/arm/mach-imx/cpuidle-imx6q.c | 27 ++++++++++-----------------
>>  1 file changed, 10 insertions(+), 17 deletions(-)
>> 
>> diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c
>> index bfeb25aaf9a2..326e870d7123 100644
>> --- a/arch/arm/mach-imx/cpuidle-imx6q.c
>> +++ b/arch/arm/mach-imx/cpuidle-imx6q.c
>> @@ -16,30 +16,23 @@
>>  #include "cpuidle.h"
>>  #include "hardware.h"
>>  
>> -static atomic_t master = ATOMIC_INIT(0);
>> -static DEFINE_SPINLOCK(master_lock);
>> +static int num_idle_cpus = 0;
>> +static DEFINE_SPINLOCK(cpuidle_lock);
>>  
>>  static int imx6q_enter_wait(struct cpuidle_device *dev,
>>  			    struct cpuidle_driver *drv, int index)
>>  {
>> -	if (atomic_inc_return(&master) == num_online_cpus()) {
>> -		/*
>> -		 * With this lock, we prevent other cpu to exit and enter
>> -		 * this function again and become the master.
>> -		 */
>> -		if (!spin_trylock(&master_lock))
>> -			goto idle;
>> +	spin_lock(&cpuidle_lock);
>> +	if (++num_idle_cpus == num_online_cpus())
>>  		imx6_set_lpm(WAIT_UNCLOCKED);
>> -		cpu_do_idle();
>> -		imx6_set_lpm(WAIT_CLOCKED);
>> -		spin_unlock(&master_lock);
>> -		goto done;
>> -	}
>> +	spin_unlock(&cpuidle_lock);
>>  
>> -idle:
>>  	cpu_do_idle();
>> -done:
>> -	atomic_dec(&master);
>> +
>> +	spin_lock(&cpuidle_lock);
>> +	if (num_idle_cpus-- == num_online_cpus())
>> +		imx6_set_lpm(WAIT_CLOCKED);
>> +	spin_unlock(&cpuidle_lock);
>>  
>>  	return index;
>>  }
>> -- 
>> 2.17.1
>> 

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

Re: [PATCH v3] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time

[Shawn Guo]

On Mon, Mar 04, 2019 at 10:28:14AM +0900, Kohji Okuno wrote:
> Hi Shawn,
> 
> I found this bug by delaying periodic communication. For example, it
> is the source codes as below.
> 
> 	while (1) {
> 		sleep(2);
> 		write(fd, "Hello", strlen("Hello"));
> 	}
> 
> Of course, it dows not occur frequently. However, in the worst case,
> it stopped for more than 1 minute.
> 
> If there are few running processes and few hrtimer events, this issue
> will occur.
> 
> For example:
> (*) time is abstracting.
>     WFI idle : the CPU will be waked up by local timer.
>     WAIT idle: the CPU will be waked up by broad cast(bc) timer.
>     
> [time]
> 	  CPU#0				  CPU#1
> ----------------------------------------------------------------------
> 	enter "WAIT" idle
> 	[next event=10]
> 					enter "WAIT" idle
> 					[next event=60]	
> 			all cpus are IDLE = > CPU#1 sets WAIT_UNCLOCKED
> 
> 10:	bc timer fires
> 	exit idle
> 	
> 	enter "WFI" idle
> 	[next event=12]
> 	
> 12:     ** local timer is not fired **
> 	CPU#0 will not be waked up
> 	until the next bc timer interrupt.
> 
> 			>> all processes stop <<
> 
> 60:	bc timer fires
> 	exit idle			exit idle
> 					CPU#1 sets WAIT_CLOCKED
> 	local timer fires
> 
> If CPU#0 sets WAIT_CLOCKED at time=10, this issue will not occur.
> 
> Does that make any sense?

Yes, makes sense, and thanks for the details.  So the fix should be
a material for stable kernel, even though the bug doesn't happen often.
Thanks for the patch.

Shawn

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

Re: [PATCH v3] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time

[Shawn Guo]

On Tue, Feb 26, 2019 at 11:34:13AM +0900, Kohji Okuno wrote:
>  In the current cpuidle implementation for i.MX6q, the CPU that sets
> 'WAIT_UNCLOCKED' and the CPU that returns to 'WAIT_CLOCKED' are always
> the same. While the CPU that sets 'WAIT_UNCLOCKED' is in IDLE state of
> "WAIT", if the other CPU wakes up and enters IDLE state of "WFI"
> istead of "WAIT", this CPU can not wake up at expired time.
>  Because, in the case of "WFI", the CPU must be waked up by the local
> timer interrupt. But, while 'WAIT_UNCLOCKED' is set, the local timer
> is stopped, when all CPUs execute "wfi" instruction. As a result, the
> local timer interrupt is not fired.
>  In this situation, this CPU will wake up by IRQ different from local
> timer. (e.g. broacast tiemr)

s/tiemr/timer

> 
> So, this fix changes CPU to return to 'WAIT_CLOCKED'.
> 
> Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>

Applied the patch with following Fixes tag.

Fixes: e5f9dec8ff5f ("ARM: imx6q: support WAIT mode using cpuidle")

Thanks for the fixing.

Shawn

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

Re: [PATCH v3] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time

[Kohji Okuno]

Hi Shawn,

Thank you for your work. I confirmed about your commit.

Best regards,
 Kohji Okuno

Shawn Guo <shawnguo@kernel.org> wrote:
> Applied the patch with following Fixes tag.
> 
> Fixes: e5f9dec8ff5f ("ARM: imx6q: support WAIT mode using cpuidle")
> 
> Thanks for the fixing.
> 
> Shawn

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

Re: [PATCH v3] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time

[Kohji Okuno]

Hi Shawn,

I found the fix of the same issue on linux-imx repository. Could you
refer to the following?

https://source.codeaurora.org/external/imx/linux-imx/commit/?h=imx_4.1.y_brillo&id=f739483488ef76979c06efea397237756bec45b9

Regarding the fix of cpuidle-imx6q.c, my fix is better, I think.
But, we may also need to modify pm-imx6.c.

# I cannot understand why they doesn't release their modifications.

Best regards,
 Kohji Okuno

Shawn Guo <shawnguo@kernel.org> wrote:
> On Tue, Feb 26, 2019 at 11:34:13AM +0900, Kohji Okuno wrote:
>>  In the current cpuidle implementation for i.MX6q, the CPU that sets
>> 'WAIT_UNCLOCKED' and the CPU that returns to 'WAIT_CLOCKED' are always
>> the same. While the CPU that sets 'WAIT_UNCLOCKED' is in IDLE state of
>> "WAIT", if the other CPU wakes up and enters IDLE state of "WFI"
>> istead of "WAIT", this CPU can not wake up at expired time.
>>  Because, in the case of "WFI", the CPU must be waked up by the local
>> timer interrupt. But, while 'WAIT_UNCLOCKED' is set, the local timer
>> is stopped, when all CPUs execute "wfi" instruction. As a result, the
>> local timer interrupt is not fired.
>>  In this situation, this CPU will wake up by IRQ different from local
>> timer. (e.g. broacast tiemr)
> 
> s/tiemr/timer
> 
>> 
>> So, this fix changes CPU to return to 'WAIT_CLOCKED'.
>> 
>> Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>
> 
> Applied the patch with following Fixes tag.
> 
> Fixes: e5f9dec8ff5f ("ARM: imx6q: support WAIT mode using cpuidle")
> 
> Thanks for the fixing.
> 
> Shawn

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

Re: [PATCH v3] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time

[Shawn Guo]

On Tue, Mar 05, 2019 at 07:38:05PM +0900, Kohji Okuno wrote:
> Hi Shawn,
> 
> I found the fix of the same issue on linux-imx repository. Could you
> refer to the following?

Thanks for the pointer.

> 
> https://source.codeaurora.org/external/imx/linux-imx/commit/?h=imx_4.1.y_brillo&id=f739483488ef76979c06efea397237756bec45b9
> 
> Regarding the fix of cpuidle-imx6q.c, my fix is better, I think.

I agree.

> But, we may also need to modify pm-imx6.c.

You can send another patch if you think the change is really necessary.

> 
> # I cannot understand why they doesn't release their modifications.

My wild guess is that they shifted the focus to new platforms like
i.MX8.

Shawn

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

[PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask of GINT

[Kohji Okuno]

In imx6_set_lpm, we only need to unmask GINT when not WAIT_CLOCKED,
so add a check condition.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>
---
 arch/arm/mach-imx/pm-imx6.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
index 87f45b926c78..54add0178b96 100644
--- a/arch/arm/mach-imx/pm-imx6.c
+++ b/arch/arm/mach-imx/pm-imx6.c
@@ -354,9 +354,11 @@ int imx6_set_lpm(enum mxc_cpu_pwr_mode mode)
 	 *
 	 * Note that IRQ #32 is GIC SPI #0.
 	 */
-	imx_gpc_hwirq_unmask(0);
+	if (mode != WAIT_CLOCKED)
+		imx_gpc_hwirq_unmask(0);
 	writel_relaxed(val, ccm_base + CLPCR);
-	imx_gpc_hwirq_mask(0);
+	if (mode != WAIT_CLOCKED)
+		imx_gpc_hwirq_mask(0);
 
 	return 0;
 }
-- 
2.17.1


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

RE: [PATCH v3] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time

[Peng Fan]

> -----Original Message-----
> From: Kohji Okuno [mailto:okuno.kohji@jp.panasonic.com]
> Sent: 2019年3月5日 18:38
> To: shawnguo@kernel.org
> Cc: linux-arm-kernel@lists.infradead.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> dl-linux-imx <linux-imx@nxp.com>; okuno.kohji@jp.panasonic.com
> Subject: Re: [PATCH v3] ARM: imx6q: cpuidle: fix bug that CPU might not wake
> up at expected time
> 
> Hi Shawn,
> 
> I found the fix of the same issue on linux-imx repository. Could you refer to
> the following?
> 
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsour
> ce.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Fcommit%2F%3Fh%3D
> imx_4.1.y_brillo%26id%3Df739483488ef76979c06efea397237756bec45b9&a
> mp;data=02%7C01%7Cpeng.fan%40nxp.com%7Cf02b29e33b2c4245a60d08d
> 6a156ad72%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636873
> 790983230223&amp;sdata=QM21UkcAUjjZq3kPdZEgrkCxvFTneGRH7%2BlQg
> GJNHoU%3D&amp;reserved=0
> 
> Regarding the fix of cpuidle-imx6q.c, my fix is better, I think.
> But, we may also need to modify pm-imx6.c.
> 
> # I cannot understand why they doesn't release their modifications.

This patch has been tried to upstream long time ago, https://lkml.org/lkml/2017/12/30/65

Regards,
Peng

> 
> Best regards,
>  Kohji Okuno
> 
> Shawn Guo <shawnguo@kernel.org> wrote:
> > On Tue, Feb 26, 2019 at 11:34:13AM +0900, Kohji Okuno wrote:
> >>  In the current cpuidle implementation for i.MX6q, the CPU that sets
> >> 'WAIT_UNCLOCKED' and the CPU that returns to 'WAIT_CLOCKED' are
> >> always the same. While the CPU that sets 'WAIT_UNCLOCKED' is in IDLE
> >> state of "WAIT", if the other CPU wakes up and enters IDLE state of "WFI"
> >> istead of "WAIT", this CPU can not wake up at expired time.
> >>  Because, in the case of "WFI", the CPU must be waked up by the local
> >> timer interrupt. But, while 'WAIT_UNCLOCKED' is set, the local timer
> >> is stopped, when all CPUs execute "wfi" instruction. As a result, the
> >> local timer interrupt is not fired.
> >>  In this situation, this CPU will wake up by IRQ different from local
> >> timer. (e.g. broacast tiemr)
> >
> > s/tiemr/timer
> >
> >>
> >> So, this fix changes CPU to return to 'WAIT_CLOCKED'.
> >>
> >> Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>
> >
> > Applied the patch with following Fixes tag.
> >
> > Fixes: e5f9dec8ff5f ("ARM: imx6q: support WAIT mode using cpuidle")
> >
> > Thanks for the fixing.
> >
> > Shawn
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] ARM: imx6q: cpuidle: fix bug that CPU might not wake up at expected time

[Kohji Okuno]

Hi Peng and Shawn,

Peng, thank you for sharing information and I'm very sorry about my
comment.

Shawn and Peng,
Could you confirm about the addition patch for pm-imx6.c?

Best regards,
 Kohji Okuno

Peng Fan <peng.fan@nxp.com> wrote:
> 
> 
>> -----Original Message-----
>> From: Kohji Okuno [mailto:okuno.kohji@jp.panasonic.com]
>> Sent: 2019年3月5日 18:38
>> To: shawnguo@kernel.org
>> Cc: linux-arm-kernel@lists.infradead.org; s.hauer@pengutronix.de;
>> kernel@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
>> dl-linux-imx <linux-imx@nxp.com>; okuno.kohji@jp.panasonic.com
>> Subject: Re: [PATCH v3] ARM: imx6q: cpuidle: fix bug that CPU might not wake
>> up at expected time
>> 
>> Hi Shawn,
>> 
>> I found the fix of the same issue on linux-imx repository. Could you refer to
>> the following?
>> 
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsour
>> ce.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Fcommit%2F%3Fh%3D
>> imx_4.1.y_brillo%26id%3Df739483488ef76979c06efea397237756bec45b9&a
>> mp;data=02%7C01%7Cpeng.fan%40nxp.com%7Cf02b29e33b2c4245a60d08d
>> 6a156ad72%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636873
>> 790983230223&amp;sdata=QM21UkcAUjjZq3kPdZEgrkCxvFTneGRH7%2BlQg
>> GJNHoU%3D&amp;reserved=0
>> 
>> Regarding the fix of cpuidle-imx6q.c, my fix is better, I think.
>> But, we may also need to modify pm-imx6.c.
>> 
>> # I cannot understand why they doesn't release their modifications.
> 
> This patch has been tried to upstream long time ago, https://lkml.org/lkml/2017/12/30/65
> 
> Regards,
> Peng
> 
>> 
>> Best regards,
>>  Kohji Okuno
>> 
>> Shawn Guo <shawnguo@kernel.org> wrote:
>> > On Tue, Feb 26, 2019 at 11:34:13AM +0900, Kohji Okuno wrote:
>> >>  In the current cpuidle implementation for i.MX6q, the CPU that sets
>> >> 'WAIT_UNCLOCKED' and the CPU that returns to 'WAIT_CLOCKED' are
>> >> always the same. While the CPU that sets 'WAIT_UNCLOCKED' is in IDLE
>> >> state of "WAIT", if the other CPU wakes up and enters IDLE state of "WFI"
>> >> istead of "WAIT", this CPU can not wake up at expired time.
>> >>  Because, in the case of "WFI", the CPU must be waked up by the local
>> >> timer interrupt. But, while 'WAIT_UNCLOCKED' is set, the local timer
>> >> is stopped, when all CPUs execute "wfi" instruction. As a result, the
>> >> local timer interrupt is not fired.
>> >>  In this situation, this CPU will wake up by IRQ different from local
>> >> timer. (e.g. broacast tiemr)
>> >
>> > s/tiemr/timer
>> >
>> >>
>> >> So, this fix changes CPU to return to 'WAIT_CLOCKED'.
>> >>
>> >> Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>
>> >
>> > Applied the patch with following Fixes tag.
>> >
>> > Fixes: e5f9dec8ff5f ("ARM: imx6q: support WAIT mode using cpuidle")
>> >
>> > Thanks for the fixing.
>> >
>> > Shawn

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

Re: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask of GINT

[Shawn Guo]

On Wed, Mar 06, 2019 at 01:30:42PM +0900, Kohji Okuno wrote:
> In imx6_set_lpm, we only need to unmask GINT when not WAIT_CLOCKED,
> so add a check condition.

Can you elaborate the problem we have without this code change?

Shawn

> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>
> ---
>  arch/arm/mach-imx/pm-imx6.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
> index 87f45b926c78..54add0178b96 100644
> --- a/arch/arm/mach-imx/pm-imx6.c
> +++ b/arch/arm/mach-imx/pm-imx6.c
> @@ -354,9 +354,11 @@ int imx6_set_lpm(enum mxc_cpu_pwr_mode mode)
>  	 *
>  	 * Note that IRQ #32 is GIC SPI #0.
>  	 */
> -	imx_gpc_hwirq_unmask(0);
> +	if (mode != WAIT_CLOCKED)
> +		imx_gpc_hwirq_unmask(0);
>  	writel_relaxed(val, ccm_base + CLPCR);
> -	imx_gpc_hwirq_mask(0);
> +	if (mode != WAIT_CLOCKED)
> +		imx_gpc_hwirq_mask(0);
>  
>  	return 0;
>  }
> -- 
> 2.17.1
> 

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

Re: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask of GINT

[Kohji Okuno]

Hi Peng and Shawn,

This patch was made by Peng. Peng, could you explain about this?
My guess is that the patch is to eliminate the unnecessary processing.

Best regards,
 Kohji Okuno

Shawn Guo <shawnguo@kernel.org> wrote:
> On Wed, Mar 06, 2019 at 01:30:42PM +0900, Kohji Okuno wrote:
>> In imx6_set_lpm, we only need to unmask GINT when not WAIT_CLOCKED,
>> so add a check condition.
> 
> Can you elaborate the problem we have without this code change?
> 
> Shawn
> 
>> 
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>
>> ---
>>  arch/arm/mach-imx/pm-imx6.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
>> index 87f45b926c78..54add0178b96 100644
>> --- a/arch/arm/mach-imx/pm-imx6.c
>> +++ b/arch/arm/mach-imx/pm-imx6.c
>> @@ -354,9 +354,11 @@ int imx6_set_lpm(enum mxc_cpu_pwr_mode mode)
>>  	 *
>>  	 * Note that IRQ #32 is GIC SPI #0.
>>  	 */
>> -	imx_gpc_hwirq_unmask(0);
>> +	if (mode != WAIT_CLOCKED)
>> +		imx_gpc_hwirq_unmask(0);
>>  	writel_relaxed(val, ccm_base + CLPCR);
>> -	imx_gpc_hwirq_mask(0);
>> +	if (mode != WAIT_CLOCKED)
>> +		imx_gpc_hwirq_mask(0);
>>  
>>  	return 0;
>>  }
>> -- 
>> 2.17.1
>> 

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

RE: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask of GINT

[Peng Fan]

> -----Original Message-----
> From: Kohji Okuno [mailto:okuno.kohji@jp.panasonic.com]
> Sent: 2019年3月20日 8:07
> To: shawnguo@kernel.org; Peng Fan <peng.fan@nxp.com>
> Cc: linux-arm-kernel@lists.infradead.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx
> <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> okuno.kohji@jp.panasonic.com
> Subject: Re: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask of
> GINT
> 
> Hi Peng and Shawn,
> 
> This patch was made by Peng. Peng, could you explain about this?
> My guess is that the patch is to eliminate the unnecessary processing.

Yes. This is optimization, there is no need to unmask/mask when
WAIT_CLOCKED. See the errata description.
	/*
	 * ERR007265: CCM: When improper low-power sequence is used,
	 * the SoC enters low power mode before the ARM core executes WFI.
	 *
	 * Software workaround:
	 * 1) Software should trigger IRQ #32 (IOMUX) to be always pending
	 *    by setting IOMUX_GPR1_GINT.
	 * 2) Software should then unmask IRQ #32 in GPC before setting CCM
	 *    Low-Power mode.
	 * 3) Software should mask IRQ #32 right after CCM Low-Power mode
	 *    is set (set bits 0-1 of CCM_CLPCR).
	 *
	 * Note that IRQ #32 is GIC SPI #0.
	 */
Regards,
Peng.

> 
> Best regards,
>  Kohji Okuno
> 
> Shawn Guo <shawnguo@kernel.org> wrote:
> > On Wed, Mar 06, 2019 at 01:30:42PM +0900, Kohji Okuno wrote:
> >> In imx6_set_lpm, we only need to unmask GINT when not
> WAIT_CLOCKED,
> >> so add a check condition.
> >
> > Can you elaborate the problem we have without this code change?
> >
> > Shawn
> >
> >>
> >> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >> Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>
> >> ---
> >>  arch/arm/mach-imx/pm-imx6.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-imx/pm-imx6.c
> >> b/arch/arm/mach-imx/pm-imx6.c index 87f45b926c78..54add0178b96
> 100644
> >> --- a/arch/arm/mach-imx/pm-imx6.c
> >> +++ b/arch/arm/mach-imx/pm-imx6.c
> >> @@ -354,9 +354,11 @@ int imx6_set_lpm(enum mxc_cpu_pwr_mode
> mode)
> >>  	 *
> >>  	 * Note that IRQ #32 is GIC SPI #0.
> >>  	 */
> >> -	imx_gpc_hwirq_unmask(0);
> >> +	if (mode != WAIT_CLOCKED)
> >> +		imx_gpc_hwirq_unmask(0);
> >>  	writel_relaxed(val, ccm_base + CLPCR);
> >> -	imx_gpc_hwirq_mask(0);
> >> +	if (mode != WAIT_CLOCKED)
> >> +		imx_gpc_hwirq_mask(0);
> >>
> >>  	return 0;
> >>  }
> >> --
> >> 2.17.1
> >>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask of GINT

[Aisheng Dong]

> From: Peng Fan
> > -----Original Message-----
> > From: Kohji Okuno [mailto:okuno.kohji@jp.panasonic.com]
> > Subject: Re: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask
> > of GINT
> >
> > Hi Peng and Shawn,
> >
> > This patch was made by Peng. Peng, could you explain about this?
> > My guess is that the patch is to eliminate the unnecessary processing.
> 
> Yes. This is optimization, there is no need to unmask/mask when
> WAIT_CLOCKED. See the errata description.
> 	/*
> 	 * ERR007265: CCM: When improper low-power sequence is used,
> 	 * the SoC enters low power mode before the ARM core executes WFI.
> 	 *
> 	 * Software workaround:
> 	 * 1) Software should trigger IRQ #32 (IOMUX) to be always pending
> 	 *    by setting IOMUX_GPR1_GINT.
> 	 * 2) Software should then unmask IRQ #32 in GPC before setting CCM
> 	 *    Low-Power mode.
> 	 * 3) Software should mask IRQ #32 right after CCM Low-Power mode
> 	 *    is set (set bits 0-1 of CCM_CLPCR).
> 	 *
> 	 * Note that IRQ #32 is GIC SPI #0.
> 	 */

Your code comments did not mention it's only for WAIT_CLOCKED.
For errata, I saw it seems also affect STOP mode.
Can you explain a bit more on why it's safe for all other modes except WAIT_CLOCKED?

Regards
Dong Aisheng

> Regards,
> Peng.
> 
> >
> > Best regards,
> >  Kohji Okuno
> >
> > Shawn Guo <shawnguo@kernel.org> wrote:
> > > On Wed, Mar 06, 2019 at 01:30:42PM +0900, Kohji Okuno wrote:
> > >> In imx6_set_lpm, we only need to unmask GINT when not
> > WAIT_CLOCKED,
> > >> so add a check condition.
> > >
> > > Can you elaborate the problem we have without this code change?
> > >
> > > Shawn
> > >
> > >>
> > >> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > >> Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>
> > >> ---
> > >>  arch/arm/mach-imx/pm-imx6.c | 6 ++++--
> > >>  1 file changed, 4 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/arch/arm/mach-imx/pm-imx6.c
> > >> b/arch/arm/mach-imx/pm-imx6.c index 87f45b926c78..54add0178b96
> > 100644
> > >> --- a/arch/arm/mach-imx/pm-imx6.c
> > >> +++ b/arch/arm/mach-imx/pm-imx6.c
> > >> @@ -354,9 +354,11 @@ int imx6_set_lpm(enum mxc_cpu_pwr_mode
> > mode)
> > >>  	 *
> > >>  	 * Note that IRQ #32 is GIC SPI #0.
> > >>  	 */
> > >> -	imx_gpc_hwirq_unmask(0);
> > >> +	if (mode != WAIT_CLOCKED)
> > >> +		imx_gpc_hwirq_unmask(0);
> > >>  	writel_relaxed(val, ccm_base + CLPCR);
> > >> -	imx_gpc_hwirq_mask(0);
> > >> +	if (mode != WAIT_CLOCKED)
> > >> +		imx_gpc_hwirq_mask(0);
> > >>
> > >>  	return 0;
> > >>  }
> > >> --
> > >> 2.17.1
> > >>

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

Re: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask of GINT

[Shawn Guo]

On Wed, Mar 20, 2019 at 01:12:01AM +0000, Peng Fan wrote:
> 
> 
> > -----Original Message-----
> > From: Kohji Okuno [mailto:okuno.kohji@jp.panasonic.com]
> > Sent: 2019年3月20日 8:07
> > To: shawnguo@kernel.org; Peng Fan <peng.fan@nxp.com>
> > Cc: linux-arm-kernel@lists.infradead.org; s.hauer@pengutronix.de;
> > kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx
> > <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> > okuno.kohji@jp.panasonic.com
> > Subject: Re: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask of
> > GINT
> > 
> > Hi Peng and Shawn,
> > 
> > This patch was made by Peng. Peng, could you explain about this?
> > My guess is that the patch is to eliminate the unnecessary processing.
> 
> Yes. This is optimization, there is no need to unmask/mask when
> WAIT_CLOCKED. See the errata description.
> 	/*
> 	 * ERR007265: CCM: When improper low-power sequence is used,
> 	 * the SoC enters low power mode before the ARM core executes WFI.
> 	 *
> 	 * Software workaround:
> 	 * 1) Software should trigger IRQ #32 (IOMUX) to be always pending
> 	 *    by setting IOMUX_GPR1_GINT.
> 	 * 2) Software should then unmask IRQ #32 in GPC before setting CCM
> 	 *    Low-Power mode.
> 	 * 3) Software should mask IRQ #32 right after CCM Low-Power mode
> 	 *    is set (set bits 0-1 of CCM_CLPCR).
> 	 *
> 	 * Note that IRQ #32 is GIC SPI #0.
> 	 */

So are you saying the workaround of ERR007265 does not need to apply on
WAIT_CLOCKED mode?  But WAIT_CLOCKED is one of low-power modes, isn't
it?

Shawn

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

RE: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask of GINT

[Peng Fan]

Hi Shawn,

> -----Original Message-----
> From: Shawn Guo [mailto:shawnguo@kernel.org]
> Sent: 2019年3月20日 15:45
> To: Peng Fan <peng.fan@nxp.com>
> Cc: Kohji Okuno <okuno.kohji@jp.panasonic.com>;
> linux-arm-kernel@lists.infradead.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask of
> GINT
> 
> On Wed, Mar 20, 2019 at 01:12:01AM +0000, Peng Fan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Kohji Okuno [mailto:okuno.kohji@jp.panasonic.com]
> > > Sent: 2019年3月20日 8:07
> > > To: shawnguo@kernel.org; Peng Fan <peng.fan@nxp.com>
> > > Cc: linux-arm-kernel@lists.infradead.org; s.hauer@pengutronix.de;
> > > kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx
> > > <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> > > okuno.kohji@jp.panasonic.com
> > > Subject: Re: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask
> > > of GINT
> > >
> > > Hi Peng and Shawn,
> > >
> > > This patch was made by Peng. Peng, could you explain about this?
> > > My guess is that the patch is to eliminate the unnecessary processing.
> >
> > Yes. This is optimization, there is no need to unmask/mask when
> > WAIT_CLOCKED. See the errata description.
> > 	/*
> > 	 * ERR007265: CCM: When improper low-power sequence is used,
> > 	 * the SoC enters low power mode before the ARM core executes WFI.
> > 	 *
> > 	 * Software workaround:
> > 	 * 1) Software should trigger IRQ #32 (IOMUX) to be always pending
> > 	 *    by setting IOMUX_GPR1_GINT.
> > 	 * 2) Software should then unmask IRQ #32 in GPC before setting CCM
> > 	 *    Low-Power mode.
> > 	 * 3) Software should mask IRQ #32 right after CCM Low-Power mode
> > 	 *    is set (set bits 0-1 of CCM_CLPCR).
> > 	 *
> > 	 * Note that IRQ #32 is GIC SPI #0.
> > 	 */
> 
> So are you saying the workaround of ERR007265 does not need to apply on
> WAIT_CLOCKED mode?  But WAIT_CLOCKED is one of low-power modes,
> isn't it?

If understand correct, I think WAIT_CLOCKED is for run mode, not wait/stop mode.

According to i.MX6Q RM, "18.6.18 CCM Low Power Control Register (CCM_CLPCR)"

Setting the low power mode that system will enter on next assertion of dsm_request signal.
NOTE: Set CCM_CGPR[INT_MEM_CLK_LPM] and CCM_CGPR[1] bits to 1 when setting
CCM_CLPCR[LPM] bits to 01 (WAIT Mode) or 10 (STOP mode) without power gating.
CCM_CGPR[INT_MEM_CLK_LPM] and CCM_CGPR[1] bits do not have to be set for STOP
mode entry.
00 Remain in run mode
01 Transfer to wait mode
10 Transfer to stop mode
11 Reserved

And according to the function:
int imx6q_set_lpm(enum mxc_cpu_pwr_mode mode)
{
        u32 val = readl_relaxed(ccm_base + CLPCR);

        val &= ~BM_CLPCR_LPM;
        switch (mode) {
        case WAIT_CLOCKED:
                break;

WAIT_CLOCKED will clear the LPM bits.


According to https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf
"
ERR007265 CCM: When improper low-power sequence is used, the SoC enters
low power mode before the ARM core executes WFI

When software tries to enter Low-Power mode with the following sequence, the SoC enters
Low-Power mode before the ARM core executes the WFI instruction:
1. Set CCM_CLPCR[1:0] to 2’b00
2. ARM core enters WFI
3. ARM core wakeup from an interrupt event, which is masked by GPC or not visible to GPC,
such as an interrupt from a local timer
4. Set CCM_CLPCR[1:0] to 2’b01 or 2’b10
5. ARM core executes WFI
Before the last step, the SoC enters WAIT mode if CCM_CLPCR[1:0] is set to 2’b01, or STOP
mode if CCM_CLPCR[1:0] is set to 2’b10.
"

It mentions that soc will enter wait/stop mode early before wfi if set CCM_CLPCR to 1/2.
But WAIT_CLOCKED is not wait mode or stop mode, it will set LPM to run mode.

Regards,
Peng.

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

Re: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask of GINT

[Shawn Guo]

Hi Peng,

On Wed, Mar 20, 2019 at 07:59:19AM +0000, Peng Fan wrote:
> > So are you saying the workaround of ERR007265 does not need to apply on
> > WAIT_CLOCKED mode?  But WAIT_CLOCKED is one of low-power modes,
> > isn't it?
> 
> If understand correct, I think WAIT_CLOCKED is for run mode, not wait/stop mode.
> 
> According to i.MX6Q RM, "18.6.18 CCM Low Power Control Register (CCM_CLPCR)"
> 
> Setting the low power mode that system will enter on next assertion of dsm_request signal.
> NOTE: Set CCM_CGPR[INT_MEM_CLK_LPM] and CCM_CGPR[1] bits to 1 when setting
> CCM_CLPCR[LPM] bits to 01 (WAIT Mode) or 10 (STOP mode) without power gating.
> CCM_CGPR[INT_MEM_CLK_LPM] and CCM_CGPR[1] bits do not have to be set for STOP
> mode entry.
> 00 Remain in run mode
> 01 Transfer to wait mode
> 10 Transfer to stop mode
> 11 Reserved
> 
> And according to the function:
> int imx6q_set_lpm(enum mxc_cpu_pwr_mode mode)
> {
>         u32 val = readl_relaxed(ccm_base + CLPCR);
> 
>         val &= ~BM_CLPCR_LPM;
>         switch (mode) {
>         case WAIT_CLOCKED:
>                 break;
> 
> WAIT_CLOCKED will clear the LPM bits.
> 
> 
> According to https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf
> "
> ERR007265 CCM: When improper low-power sequence is used, the SoC enters
> low power mode before the ARM core executes WFI
> 
> When software tries to enter Low-Power mode with the following sequence, the SoC enters
> Low-Power mode before the ARM core executes the WFI instruction:
> 1. Set CCM_CLPCR[1:0] to 2’b00
> 2. ARM core enters WFI
> 3. ARM core wakeup from an interrupt event, which is masked by GPC or not visible to GPC,
> such as an interrupt from a local timer
> 4. Set CCM_CLPCR[1:0] to 2’b01 or 2’b10
> 5. ARM core executes WFI
> Before the last step, the SoC enters WAIT mode if CCM_CLPCR[1:0] is set to 2’b01, or STOP
> mode if CCM_CLPCR[1:0] is set to 2’b10.
> "
> 
> It mentions that soc will enter wait/stop mode early before wfi if set CCM_CLPCR to 1/2.
> But WAIT_CLOCKED is not wait mode or stop mode, it will set LPM to run mode.

Thanks much for the detailed explanation.  I messed WAIT_CLOCKED with
WAIT_UNCLOCKED.  Yes, I agree it's an optimization that we skip applying
the workaround to WAIT_CLOCKED.

Shawn

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

Re: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask of GINT

[Shawn Guo]

On Wed, Mar 06, 2019 at 01:30:42PM +0900, Kohji Okuno wrote:
> In imx6_set_lpm, we only need to unmask GINT when not WAIT_CLOCKED,
> so add a check condition.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com>

Applied, thanks.

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