ARM: zynq: wfi exit on same cpu is valid

PATCH New Sanjay Singh Rawat 5 participants 17 messages May 31, 2013 – Jun 5, 2013 1 message not in the archive

Subsystems (3)

Arm (32-bit) Arm sub-architectures Arm/Zynq architecture

lore

Share & tools

lore

Sanjay Singh Rawat

author

2013-05-31 10:44 UTC #0 lore raw

PATCH 1/1 ARM: zynq: wfi exit on same cpu is valid

The current code considers every wakeup as spurious, which is not correct. Handle the same way as other arm platforms are doing.

Signed-off-by Sanjay Singh Rawat <[email protected]>

---
 arch/arm/mach-zynq/hotplug.c |    7 +++++++
 1 file changed, 7 insertions(+)

arch/arm/mach-zynq/hotplug.c +7 -0

index c89672b..a1ab22c 100644
@@ -67,6 +67,13 @@ static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious)
 		dsb();
 		wfi();
 
+		if (pen_release == cpu_logical_map(cpu)) {
+			/*
+			 * OK, proper wakeup, we're done
+			 */
+			break;
+		}
+
 		/*
 		 * Getting here, means that we have come out of WFI without
 		 * having been woken up - this shouldn't happen

Signature · 1 line

1.7.9.5

Daniel Lezcano

2013-06-03 08:14 UTC #1 lore raw

PATCH 1/1 ARM: zynq: wfi exit on same cpu is valid

Quoted 4 lines · Sanjay Singh Rawat

The current code considers every wakeup as spurious, which is not correct. Handle the same way as other arm platforms are doing.
Signed-off-by Sanjay Singh Rawat <[email protected]>

Reviewed-by Daniel Lezcano <[email protected]>

Quoted 23 lines

---
 arch/arm/mach-zynq/hotplug.c |    7 +++++++
 1 file changed, 7 insertions(+)

arch/arm/mach-zynq/hotplug.c +7 -0

index c89672b..a1ab22c 100644
@@ -67,6 +67,13 @@ static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious)
 		dsb();
 		wfi();
 
+		if (pen_release == cpu_logical_map(cpu)) {
+			/*
+			 * OK, proper wakeup, we're done
+			 */
+			break;
+		}
+
 		/*
 		 * Getting here, means that we have come out of WFI without
 		 * having been woken up - this shouldn't happen

Signature · 5 lines

 <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

Michal Simek

2013-06-03 09:51 UTC #2 lore raw

PATCH 1/1 ARM: zynq: wfi exit on same cpu is valid

Hi,

Quoted 21 lines · Daniel Lezcano

Quoted 4 lines · Sanjay Singh Rawat
The current code considers every wakeup as spurious, which is not correct. Handle the same way as other arm platforms are doing.
Signed-off-by Sanjay Singh Rawat <[email protected]>
Reviewed-by Daniel Lezcano <[email protected]>
Quoted 12 lines
---
 arch/arm/mach-zynq/hotplug.c |    7 +++++++
 1 file changed, 7 insertions(+)
arch/arm/mach-zynq/hotplug.c +0 -0
index c89672b..a1ab22c 100644
@@ -67,6 +67,13 @@ static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious)
 		dsb();
 		wfi();
 

Quoted 7 lines

arch/arm/mach-zynq/hotplug.c static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious) · line 76
Quoted 7 lines
arch/arm/mach-zynq/hotplug.c +7 -0
+		if (pen_release == cpu_logical_map(cpu)) {
+			/*
+			 * OK, proper wakeup, we're done
+			 */
+			break;
+		}
+

what pen_release stands for?
I have looked at it and others platform are also using it in platsmp code which is not zynq case.
How is it supposed to work and how this variable should be used?

Thanks,
Michal

Signature · 14 lines

Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130603/0494cd1f/attachment-0001.sig>

Daniel Lezcano

2013-06-03 12:43 UTC #3 lore raw

PATCH 1/1 ARM: zynq: wfi exit on same cpu is valid

Quoted 23 lines · Michal Simek

Hi,
Quoted 20 lines · Daniel Lezcano
Quoted 4 lines · Sanjay Singh Rawat
The current code considers every wakeup as spurious, which is not correct. Handle the same way as other arm platforms are doing.
Signed-off-by Sanjay Singh Rawat <[email protected]>
Reviewed-by Daniel Lezcano <[email protected]>
Quoted 12 lines
---
 arch/arm/mach-zynq/hotplug.c |    7 +++++++
 1 file changed, 7 insertions(+)
arch/arm/mach-zynq/hotplug.c +0 -0
index c89672b..a1ab22c 100644
@@ -67,6 +67,13 @@ static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious)
 		dsb();
 		wfi();
 

Quoted 11 lines

Quoted 7 lines
arch/arm/mach-zynq/hotplug.c static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious) · line 76
Quoted 7 lines
arch/arm/mach-zynq/hotplug.c +7 -0
+		if (pen_release == cpu_logical_map(cpu)) {
+			/*
+			 * OK, proper wakeup, we're done
+			 */
+			break;
+		}
+
what pen_release stands for?
I have looked at it and others platform are also using it in platsmp code which is not zynq case.
How is it supposed to work and how this variable should be used?

This variable is used to serialize the secondary cpus boot process.

When the processors boot, all the processors except the CPU0 are in WFI.

Then it is up to CPU0 to wake up the secondary processors:
1. it writes the cpu id of the secondary processor in the pen_release variable
2. it sends a IPI to the secondary cpu in order to make it exiting the WFI mode
2.1 the secondary processor boots and, when finished, writes the value '-1' in the pen_release variable
3. meanwhile CPU0 waits for the pen_release to be '-1' before continuing to boot the other secondary processors

In the case of the routine "zynq_platform_do_lowpower", the same sequence occurs with 'cpu_up', that means you are at the beginning of the boot up sequence. So the pen_release value is the cpu id value when exiting from WFI (remember it sets to -1 when finished).

If the cpu exits the lopower mode but the pen_release is not the cpu id, that means there is something wrong because the cpu exited the WFI mode without being in the booting process (the cpu shouldn't receive an hw interrupt because they should have been migrated, but just a wake up IPI). This is why there's "spurious".

The pen_release contains the hardware id of the CPU, this is why cpu_logical_map is used.

I hope that helps

-- Daniel

ps : sorry for my bad English :)

Signature · 5 lines

 <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

Michal Simek

2013-06-04 07:29 UTC #4 lore raw

PATCH 1/1 ARM: zynq: wfi exit on same cpu is valid

Quoted 61 lines · Daniel Lezcano

Quoted 23 lines · Michal Simek
Hi,
Quoted 20 lines · Daniel Lezcano
Quoted 4 lines · Sanjay Singh Rawat
The current code considers every wakeup as spurious, which is not correct. Handle the same way as other arm platforms are doing.
Signed-off-by Sanjay Singh Rawat <[email protected]>
Reviewed-by Daniel Lezcano <[email protected]>
Quoted 12 lines
---
 arch/arm/mach-zynq/hotplug.c |    7 +++++++
 1 file changed, 7 insertions(+)
arch/arm/mach-zynq/hotplug.c +0 -0
index c89672b..a1ab22c 100644
@@ -67,6 +67,13 @@ static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious)
 		dsb();
 		wfi();
 
Quoted 11 lines
Quoted 7 lines
arch/arm/mach-zynq/hotplug.c static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious) · line 76
Quoted 7 lines
arch/arm/mach-zynq/hotplug.c +7 -0
+		if (pen_release == cpu_logical_map(cpu)) {
+			/*
+			 * OK, proper wakeup, we're done
+			 */
+			break;
+		}
+
what pen_release stands for?
I have looked at it and others platform are also using it in platsmp code which is not zynq case.
How is it supposed to work and how this variable should be used?
This variable is used to serialize the secondary cpus boot process.
When the processors boot, all the processors except the CPU0 are in WFI.
Then it is up to CPU0 to wake up the secondary processors:
1. it writes the cpu id of the secondary processor in the pen_release variable
2. it sends a IPI to the secondary cpu in order to make it exiting the WFI mode
2.1 the secondary processor boots and, when finished, writes the value '-1' in the pen_release variable
3. meanwhile CPU0 waits for the pen_release to be '-1' before continuing to boot the other secondary processors
In the case of the routine "zynq_platform_do_lowpower", the same sequence occurs with 'cpu_up', that means you are at the beginning of the boot up sequence. So the pen_release value is the cpu id value when exiting from WFI (remember it sets to -1 when finished).
If the cpu exits the lopower mode but the pen_release is not the cpu id, that means there is something wrong because the cpu exited the WFI mode without being in the booting process (the cpu shouldn't receive an hw interrupt because they should have been migrated, but just a wake up IPI). This is why there's "spurious".

Quoted 4 lines

The pen_release contains the hardware id of the CPU, this is why cpu_logical_map is used.
I hope that helps

Thanks for explanation.
I have added this patch to zynq/cleanup branch.

Quoted 3 lines

-- Daniel
ps : sorry for my bad English :)

No reason to apologise.

Thanks,
Michal

Signature · 14 lines

Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130604/be013185/attachment-0001.sig>

Daniel Lezcano

2013-06-04 12:40 UTC #5 lore raw

PATCH 1/1 ARM: zynq: wfi exit on same cpu is valid

Quoted 62 lines · Michal Simek

Quoted 61 lines · Daniel Lezcano
Quoted 23 lines · Michal Simek
Hi,
Quoted 20 lines · Daniel Lezcano
Quoted 4 lines · Sanjay Singh Rawat
The current code considers every wakeup as spurious, which is not correct. Handle the same way as other arm platforms are doing.
Signed-off-by Sanjay Singh Rawat <[email protected]>
Reviewed-by Daniel Lezcano <[email protected]>
Quoted 12 lines
---
 arch/arm/mach-zynq/hotplug.c |    7 +++++++
 1 file changed, 7 insertions(+)
arch/arm/mach-zynq/hotplug.c +0 -0
index c89672b..a1ab22c 100644
@@ -67,6 +67,13 @@ static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious)
 		dsb();
 		wfi();
 
Quoted 11 lines
Quoted 7 lines
arch/arm/mach-zynq/hotplug.c static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious) · line 76
Quoted 7 lines
arch/arm/mach-zynq/hotplug.c +7 -0
+		if (pen_release == cpu_logical_map(cpu)) {
+			/*
+			 * OK, proper wakeup, we're done
+			 */
+			break;
+		}
+
what pen_release stands for?
I have looked at it and others platform are also using it in platsmp code which is not zynq case.
How is it supposed to work and how this variable should be used?
This variable is used to serialize the secondary cpus boot process.
When the processors boot, all the processors except the CPU0 are in WFI.
Then it is up to CPU0 to wake up the secondary processors:
1. it writes the cpu id of the secondary processor in the pen_release variable
2. it sends a IPI to the secondary cpu in order to make it exiting the WFI mode
2.1 the secondary processor boots and, when finished, writes the value '-1' in the pen_release variable
3. meanwhile CPU0 waits for the pen_release to be '-1' before continuing to boot the other secondary processors
In the case of the routine "zynq_platform_do_lowpower", the same sequence occurs with 'cpu_up', that means you are at the beginning of the boot up sequence. So the pen_release value is the cpu id value when exiting from WFI (remember it sets to -1 when finished).
If the cpu exits the lopower mode but the pen_release is not the cpu id, that means there is something wrong because the cpu exited the WFI mode without being in the booting process (the cpu shouldn't receive an hw interrupt because they should have been migrated, but just a wake up IPI). This is why there's "spurious".

Quoted 7 lines

Quoted 4 lines
The pen_release contains the hardware id of the CPU, this is why cpu_logical_map is used.
I hope that helps
Thanks for explanation.
I have added this patch to zynq/cleanup branch.

Actually, I noticed in the boot secondary cpu the pen_release is not initialized at all. The patch is correct but with undefined behavior.

Normally, the secondary cpu are powered on and stays in WFI or whatever more power efficient but the algorithm is to initialize the pen_release with the cpu id physical id, send an IPI and wait for the pen_release to be equal to -1.

There is a good example in mach-ux500/platform.c

And a good explanation in the "Cortex -A Series programmer's guide" page 23-10.

Regards
-- Daniel

Signature · 5 lines

 <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

Michal Simek

2013-06-04 13:09 UTC #6 lore raw

PATCH 1/1 ARM: zynq: wfi exit on same cpu is valid

Quoted 66 lines · Daniel Lezcano

Quoted 65 lines · Michal Simek
Quoted 64 lines · Daniel Lezcano
Quoted 23 lines · Michal Simek
Hi,
Quoted 20 lines · Daniel Lezcano
Quoted 4 lines · Sanjay Singh Rawat
The current code considers every wakeup as spurious, which is not correct. Handle the same way as other arm platforms are doing.
Signed-off-by Sanjay Singh Rawat <[email protected]>
Reviewed-by Daniel Lezcano <[email protected]>
Quoted 12 lines
---
 arch/arm/mach-zynq/hotplug.c |    7 +++++++
 1 file changed, 7 insertions(+)
arch/arm/mach-zynq/hotplug.c +0 -0
index c89672b..a1ab22c 100644
@@ -67,6 +67,13 @@ static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious)
 		dsb();
 		wfi();
 
Quoted 11 lines
Quoted 7 lines
arch/arm/mach-zynq/hotplug.c static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious) · line 76
Quoted 7 lines
arch/arm/mach-zynq/hotplug.c +7 -0
+		if (pen_release == cpu_logical_map(cpu)) {
+			/*
+			 * OK, proper wakeup, we're done
+			 */
+			break;
+		}
+
what pen_release stands for?
I have looked at it and others platform are also using it in platsmp code which is not zynq case.
How is it supposed to work and how this variable should be used?
This variable is used to serialize the secondary cpus boot process.
When the processors boot, all the processors except the CPU0 are in WFI.
Then it is up to CPU0 to wake up the secondary processors:
1. it writes the cpu id of the secondary processor in the pen_release variable
2. it sends a IPI to the secondary cpu in order to make it exiting the WFI mode
2.1 the secondary processor boots and, when finished, writes the value '-1' in the pen_release variable
3. meanwhile CPU0 waits for the pen_release to be '-1' before continuing to boot the other secondary processors
In the case of the routine "zynq_platform_do_lowpower", the same sequence occurs with 'cpu_up', that means you are at the beginning of the boot up sequence. So the pen_release value is the cpu id value when exiting from WFI (remember it sets to -1 when finished).
If the cpu exits the lopower mode but the pen_release is not the cpu id, that means there is something wrong because the cpu exited the WFI mode without being in the booting process (the cpu shouldn't receive an hw interrupt because they should have been migrated, but just a wake up IPI). This is why there's "spurious".
The pen_release contains the hardware id of the CPU, this is why cpu_logical_map is used.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
I hope that helps
Thanks for explanation.
I have added this patch to zynq/cleanup branch.
Actually, I noticed in the boot secondary cpu the pen_release is not initialized at all. The patch is correct but with undefined behavior.

I know, it is not changed.
__cpu_logical_map is initialized as 0 in setup.c
pen_release as -1 in smp.c

Quoted 9 lines

Normally, the secondary cpu are powered on and stays in WFI or whatever more power efficient but the algorithm is to initialize the pen_release with the cpu id physical id, send an IPI and wait for the pen_release to be equal to -1.
There is a good example in mach-ux500/platform.c
And a good explanation in the "Cortex -A Series programmer's guide" page 23-10.

Where exactly cpus are waiting in WFI loop?

Thanks,
Michal

Signature · 14 lines

Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130604/2ecd24c5/attachment-0001.sig>

Daniel Lezcano

2013-06-04 13:25 UTC #7 lore raw

PATCH 1/1 ARM: zynq: wfi exit on same cpu is valid

Quoted 80 lines · Michal Simek

Quoted 62 lines · Daniel Lezcano
Quoted 61 lines · Michal Simek
Quoted 60 lines · Daniel Lezcano
Quoted 23 lines · Michal Simek
Hi,
Quoted 20 lines · Daniel Lezcano
Quoted 4 lines · Sanjay Singh Rawat
The current code considers every wakeup as spurious, which is not correct. Handle the same way as other arm platforms are doing.
Signed-off-by Sanjay Singh Rawat <[email protected]>
Reviewed-by Daniel Lezcano <[email protected]>
Quoted 12 lines
---
 arch/arm/mach-zynq/hotplug.c |    7 +++++++
 1 file changed, 7 insertions(+)
arch/arm/mach-zynq/hotplug.c +0 -0
index c89672b..a1ab22c 100644
@@ -67,6 +67,13 @@ static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious)
 		dsb();
 		wfi();
 
Quoted 11 lines
Quoted 7 lines
arch/arm/mach-zynq/hotplug.c static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious) · line 76
Quoted 7 lines
arch/arm/mach-zynq/hotplug.c +7 -0
+		if (pen_release == cpu_logical_map(cpu)) {
+			/*
+			 * OK, proper wakeup, we're done
+			 */
+			break;
+		}
+
what pen_release stands for?
I have looked at it and others platform are also using it in platsmp code which is not zynq case.
How is it supposed to work and how this variable should be used?
This variable is used to serialize the secondary cpus boot process.
When the processors boot, all the processors except the CPU0 are in WFI.
Then it is up to CPU0 to wake up the secondary processors:
1. it writes the cpu id of the secondary processor in the pen_release variable
2. it sends a IPI to the secondary cpu in order to make it exiting the WFI mode
2.1 the secondary processor boots and, when finished, writes the value '-1' in the pen_release variable
3. meanwhile CPU0 waits for the pen_release to be '-1' before continuing to boot the other secondary processors
In the case of the routine "zynq_platform_do_lowpower", the same sequence occurs with 'cpu_up', that means you are at the beginning of the boot up sequence. So the pen_release value is the cpu id value when exiting from WFI (remember it sets to -1 when finished).
If the cpu exits the lopower mode but the pen_release is not the cpu id, that means there is something wrong because the cpu exited the WFI mode without being in the booting process (the cpu shouldn't receive an hw interrupt because they should have been migrated, but just a wake up IPI). This is why there's "spurious".
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
The pen_release contains the hardware id of the CPU, this is why cpu_logical_map is used.
I hope that helps
Thanks for explanation.
I have added this patch to zynq/cleanup branch.
Actually, I noticed in the boot secondary cpu the pen_release is not initialized at all. The patch is correct but with undefined behavior.
I know, it is not changed.
__cpu_logical_map is initialized as 0 in setup.c
pen_release as -1 in smp.c
Quoted 5 lines
Normally, the secondary cpu are powered on and stays in WFI or whatever more power efficient but the algorithm is to initialize the pen_release with the cpu id physical id, send an IPI and wait for the pen_release to be equal to -1.

Quoted 5 lines

Quoted 4 lines
There is a good example in mach-ux500/platform.c
And a good explanation in the "Cortex -A Series programmer's guide" page 23-10.
Where exactly cpus are waiting in WFI loop?

When the cpu is booted, it runs the idle thread with its idle loop which calls the cpu_do_idle (WFI).

Amit Kucheria

2013-06-04 11:39 UTC #8 lore raw

PATCH 1/1 ARM: zynq: wfi exit on same cpu is valid

In reply to Daniel Lezcano

Quoted 36 lines · Daniel Lezcano

Quoted 23 lines · Michal Simek
Hi,
Quoted 20 lines · Daniel Lezcano
Quoted 4 lines · Sanjay Singh Rawat
The current code considers every wakeup as spurious, which is not correct. Handle the same way as other arm platforms are doing.
Signed-off-by Sanjay Singh Rawat <[email protected]>
Reviewed-by Daniel Lezcano <[email protected]>
Quoted 12 lines
---
 arch/arm/mach-zynq/hotplug.c |    7 +++++++
 1 file changed, 7 insertions(+)
arch/arm/mach-zynq/hotplug.c +0 -0
index c89672b..a1ab22c 100644
@@ -67,6 +67,13 @@ static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious)
            dsb();
            wfi();
 
Quoted 11 lines
Quoted 7 lines
arch/arm/mach-zynq/hotplug.c static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious) · line 76
Quoted 7 lines
arch/arm/mach-zynq/hotplug.c +7 -0
+           if (pen_release == cpu_logical_map(cpu)) {
+                   /*
+                    * OK, proper wakeup, we're done
+                    */
+                   break;
+           }
+
what pen_release stands for?
I have looked at it and others platform are also using it in platsmp code which is not zynq case.
How is it supposed to work and how this variable should be used?

Quoted 1 line

This variable is used to serialize the secondary cpus boot process.

I'm curious why it is called pen_release. :) Is there some historical link to some HW lines?

Quoted 1 line

When the processors boot, all the processors except the CPU0 are in WFI.

Daniel Lezcano

2013-06-04 11:58 UTC #9 lore raw

PATCH 1/1 ARM: zynq: wfi exit on same cpu is valid

Quoted 38 lines · Amit Kucheria

Quoted 36 lines · Daniel Lezcano
Quoted 34 lines · Michal Simek
Hi,
Quoted 20 lines · Daniel Lezcano
Quoted 4 lines · Sanjay Singh Rawat
The current code considers every wakeup as spurious, which is not correct. Handle the same way as other arm platforms are doing.
Signed-off-by Sanjay Singh Rawat <[email protected]>
Reviewed-by Daniel Lezcano <[email protected]>
Quoted 12 lines
---
 arch/arm/mach-zynq/hotplug.c |    7 +++++++
 1 file changed, 7 insertions(+)
arch/arm/mach-zynq/hotplug.c +0 -0
index c89672b..a1ab22c 100644
@@ -67,6 +67,13 @@ static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious)
            dsb();
            wfi();
 
Quoted 7 lines
arch/arm/mach-zynq/hotplug.c static inline void zynq_platform_do_lowpower(unsigned int cpu, int *spurious) · line 76
Quoted 7 lines
arch/arm/mach-zynq/hotplug.c +7 -0
+           if (pen_release == cpu_logical_map(cpu)) {
+                   /*
+                    * OK, proper wakeup, we're done
+                    */
+                   break;
+           }
+
what pen_release stands for?
I have looked at it and others platform are also using it in platsmp code which is not zynq case.
How is it supposed to work and how this variable should be used?

Quoted 4 lines

Quoted 1 line
This variable is used to serialize the secondary cpus boot process.
I'm curious why it is called pen_release. :) Is there some historical link to some HW lines?

I tried to figure out the same but I did not found any information on that. I assumed the name could be referring to a simplified mutual exclusion algorithm from the 'Dining philosophers problem' [1] where the fork is a pen.

But maybe I am totally wrong :)

[1] http://en.wikipedia.org/wiki/Dining_philosophers_problem

Signature · 5 lines

 <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

Russell King - ARM Linux

2013-06-04 14:10 UTC #10 lore raw

PATCH 1/1 ARM: zynq: wfi exit on same cpu is valid

Quoted 8 lines · Daniel Lezcano

Quoted 2 lines · Amit Kucheria
I'm curious why it is called pen_release. :) Is there some historical link to some HW lines?
I tried to figure out the same but I did not found any information on that. I assumed the name could be referring to a simplified mutual exclusion algorithm from the 'Dining philosophers problem' [1] where the fork is a pen.

Where it comes from is the original ARM SMP patches from early 2000, which everyone has blindly copied with no thought about what they're doing. This is why I'm totally against any consolidation of this code, because I'm of the opinion that _no one_ other than the ARM Ltd development platforms should be using it.

"pen" means "holding pen". It comes about because early on in the SMP development, ARM SMP platforms had four CPUs, and it was only possible to release all three secondary CPUs from the boot loader simultaneously to a common piece of code.

As the kernel was not able to serialize the release of each CPU, ARM Ltd worked around this problem by having all the CPUs jump to assembly code which "holds" the CPUs which we didn't want to boot yet, and the CPUs are released one at a time by setting pen_release to the hardware CPU number.

Modern platforms either have just one secondary CPU, or they have a way to control the reset/power to the secondary CPU. This makes the holding pen entirely redundant, and such platforms should _not_ make use of any kind of holding pen.

Russell King - ARM Linux

2013-06-04 14:17 UTC #11 lore raw

PATCH 1/1 ARM: zynq: wfi exit on same cpu is valid

Quoted 27 lines · Russell King - ARM Linux

Quoted 8 lines · Daniel Lezcano
Quoted 2 lines · Amit Kucheria
I'm curious why it is called pen_release. :) Is there some historical link to some HW lines?
I tried to figure out the same but I did not found any information on that. I assumed the name could be referring to a simplified mutual exclusion algorithm from the 'Dining philosophers problem' [1] where the fork is a pen.
Where it comes from is the original ARM SMP patches from early 2000, which everyone has blindly copied with no thought about what they're doing. This is why I'm totally against any consolidation of this code, because I'm of the opinion that _no one_ other than the ARM Ltd development platforms should be using it.
"pen" means "holding pen". It comes about because early on in the SMP development, ARM SMP platforms had four CPUs, and it was only possible to release all three secondary CPUs from the boot loader simultaneously to a common piece of code.
As the kernel was not able to serialize the release of each CPU, ARM Ltd worked around this problem by having all the CPUs jump to assembly code which "holds" the CPUs which we didn't want to boot yet, and the CPUs are released one at a time by setting pen_release to the hardware CPU number.

Quoted 4 lines

Modern platforms either have just one secondary CPU, or they have a way to control the reset/power to the secondary CPU. This makes the holding pen entirely redundant, and such platforms should _not_ make use of any kind of holding pen.

And yes, indeed, zynq can control the secondary CPU:

void zynq_slcr_cpu_start(int cpu)

{
        /* enable CPUn */
        writel(SLCR_A9_CPU_CLKSTOP << cpu,
               zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
        /* enable CLK for CPUn */
        writel(0x0 << cpu, zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
}

void zynq_slcr_cpu_stop(int cpu)

{
        /* stop CLK and reset CPUn */
        writel((SLCR_A9_CPU_CLKSTOP | SLCR_A9_CPU_RST) << cpu,
               zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
}

So there's no need for the pen. There's no need for the low power crap in hotplug.c, there's no need for the pen in hotplug.c. You just arrange for the secondary CPU to have its clock stopped and reset when it is taken offline.

Hotplugging a CPU back in _should_ be no different from its initial bringup into the kernel.

Daniel Lezcano

2013-06-05 09:34 UTC #12 lore raw

PATCH 1/1 ARM: zynq: wfi exit on same cpu is valid

Quoted 45 lines · Russell King - ARM Linux

Quoted 27 lines · Russell King - ARM Linux
Quoted 8 lines · Daniel Lezcano
Quoted 2 lines · Amit Kucheria
I'm curious why it is called pen_release. :) Is there some historical link to some HW lines?
I tried to figure out the same but I did not found any information on that. I assumed the name could be referring to a simplified mutual exclusion algorithm from the 'Dining philosophers problem' [1] where the fork is a pen.
Where it comes from is the original ARM SMP patches from early 2000, which everyone has blindly copied with no thought about what they're doing. This is why I'm totally against any consolidation of this code, because I'm of the opinion that _no one_ other than the ARM Ltd development platforms should be using it.
"pen" means "holding pen". It comes about because early on in the SMP development, ARM SMP platforms had four CPUs, and it was only possible to release all three secondary CPUs from the boot loader simultaneously to a common piece of code.
As the kernel was not able to serialize the release of each CPU, ARM Ltd worked around this problem by having all the CPUs jump to assembly code which "holds" the CPUs which we didn't want to boot yet, and the CPUs are released one at a time by setting pen_release to the hardware CPU number.
Quoted 4 lines
Modern platforms either have just one secondary CPU, or they have a way to control the reset/power to the secondary CPU. This makes the holding pen entirely redundant, and such platforms should _not_ make use of any kind of holding pen.
And yes, indeed, zynq can control the secondary CPU:
void zynq_slcr_cpu_start(int cpu)
{
        /* enable CPUn */
        writel(SLCR_A9_CPU_CLKSTOP << cpu,
               zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
        /* enable CLK for CPUn */
        writel(0x0 << cpu, zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
}
void zynq_slcr_cpu_stop(int cpu)

Quoted 8 lines

{
        /* stop CLK and reset CPUn */
        writel((SLCR_A9_CPU_CLKSTOP | SLCR_A9_CPU_RST) << cpu,
               zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
}
So there's no need for the pen. There's no need for the low power crap in hotplug.c, there's no need for the pen in hotplug.c.

Thanks Russell, that finishes to clarify what is the pen release for or better say 'was' :)

So if I follow correctly, there are some inadequate code with different mach-* because they are using a WFI instructions instead of simply powering down the core, right ? (eg. mach-ux500/hotplug.c).

Quoted 5 lines

You just arrange
for the secondary CPU to have its clock stopped and reset when it is taken offline.
Hotplugging a CPU back in _should_ be no different from its initial bringup into the kernel.

Thanks
-- Daniel

Signature · 5 lines

 <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

Michal Simek

2013-06-05 10:47 UTC #13 lore raw

PATCH 1/1 ARM: zynq: wfi exit on same cpu is valid

In reply to Russell King - ARM Linux

Quoted 51 lines · Russell King - ARM Linux

Quoted 27 lines · Russell King - ARM Linux
Quoted 8 lines · Daniel Lezcano
Quoted 2 lines · Amit Kucheria
I'm curious why it is called pen_release. :) Is there some historical link to some HW lines?
I tried to figure out the same but I did not found any information on that. I assumed the name could be referring to a simplified mutual exclusion algorithm from the 'Dining philosophers problem' [1] where the fork is a pen.
Where it comes from is the original ARM SMP patches from early 2000, which everyone has blindly copied with no thought about what they're doing. This is why I'm totally against any consolidation of this code, because I'm of the opinion that _no one_ other than the ARM Ltd development platforms should be using it.
"pen" means "holding pen". It comes about because early on in the SMP development, ARM SMP platforms had four CPUs, and it was only possible to release all three secondary CPUs from the boot loader simultaneously to a common piece of code.
As the kernel was not able to serialize the release of each CPU, ARM Ltd worked around this problem by having all the CPUs jump to assembly code which "holds" the CPUs which we didn't want to boot yet, and the CPUs are released one at a time by setting pen_release to the hardware CPU number.
Quoted 4 lines
Modern platforms either have just one secondary CPU, or they have a way to control the reset/power to the secondary CPU. This makes the holding pen entirely redundant, and such platforms should _not_ make use of any kind of holding pen.
And yes, indeed, zynq can control the secondary CPU:
void zynq_slcr_cpu_start(int cpu)
{
        /* enable CPUn */
        writel(SLCR_A9_CPU_CLKSTOP << cpu,
               zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
        /* enable CLK for CPUn */
        writel(0x0 << cpu, zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
}
void zynq_slcr_cpu_stop(int cpu)
{
        /* stop CLK and reset CPUn */
        writel((SLCR_A9_CPU_CLKSTOP | SLCR_A9_CPU_RST) << cpu,
               zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
}

Quoted 7 lines

So there's no need for the pen. There's no need for the low power crap in hotplug.c, there's no need for the pen in hotplug.c. You just arrange for the secondary CPU to have its clock stopped and reset when it is taken offline.
Hotplugging a CPU back in _should_ be no different from its initial bringup into the kernel.

I have tested that and cpu_die code is performed on cpu which should die.
And simple calling zynq_slcr_cpu_stop() on cpu which should die just doesn't work.
There is probably any expectation which I can't see.

Feel free to suggest me proper solution.

Thanks,
Michal

Signature · 14 lines

Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130605/d7336dc4/attachment.sig>

Russell King - ARM Linux

2013-06-05 11:29 UTC #14 lore raw

PATCH 1/1 ARM: zynq: wfi exit on same cpu is valid

Quoted 24 lines · Michal Simek

Quoted 23 lines · Russell King - ARM Linux
And yes, indeed, zynq can control the secondary CPU:
void zynq_slcr_cpu_start(int cpu)
{
        /* enable CPUn */
        writel(SLCR_A9_CPU_CLKSTOP << cpu,
               zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
        /* enable CLK for CPUn */
        writel(0x0 << cpu, zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
}
void zynq_slcr_cpu_stop(int cpu)
{
        /* stop CLK and reset CPUn */
        writel((SLCR_A9_CPU_CLKSTOP | SLCR_A9_CPU_RST) << cpu,
               zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
}
So there's no need for the pen. There's no need for the low power crap in hotplug.c, there's no need for the pen in hotplug.c. You just arrange for the secondary CPU to have its clock stopped and reset when it is taken offline.

Quoted 8 lines

Quoted 2 lines
Hotplugging a CPU back in _should_ be no different from its initial bringup into the kernel.
I have tested that and cpu_die code is performed on cpu which should die.
And simple calling zynq_slcr_cpu_stop() on cpu which should die just doesn't work.
There is probably any expectation which I can't see.

Have you checked whether the CPU to die can write to this register to stop its own clock and reset itself? Maybe you need a wfi() after writing to that register to trigger the hardware to shutdown the CPU?

It's also entirely possible that you need some other CPU to do those steps while the CPU to die spins in a loop or a wfi or something like that. You can hook into that via the cpu_kill() callback, which will have the CPU which is to die as its argument.

Without knowing how your hardware works, it's very difficult to be able to positively point you in an appropriate direction, but the fact that it's allegedly possible to turn the clock off and place the secondary CPU in reset means that this should be possible somehow.

Michal Simek

2013-06-05 12:54 UTC #15 lore raw

PATCH 1/1 ARM: zynq: wfi exit on same cpu is valid

Quoted 34 lines · Russell King - ARM Linux

Quoted 24 lines · Michal Simek
Quoted 23 lines · Russell King - ARM Linux
And yes, indeed, zynq can control the secondary CPU:
void zynq_slcr_cpu_start(int cpu)
{
        /* enable CPUn */
        writel(SLCR_A9_CPU_CLKSTOP << cpu,
               zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
        /* enable CLK for CPUn */
        writel(0x0 << cpu, zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
}
void zynq_slcr_cpu_stop(int cpu)
{
        /* stop CLK and reset CPUn */
        writel((SLCR_A9_CPU_CLKSTOP | SLCR_A9_CPU_RST) << cpu,
               zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
}
So there's no need for the pen. There's no need for the low power crap in hotplug.c, there's no need for the pen in hotplug.c. You just arrange for the secondary CPU to have its clock stopped and reset when it is taken offline.
Quoted 8 lines
Quoted 2 lines
Hotplugging a CPU back in _should_ be no different from its initial bringup into the kernel.
I have tested that and cpu_die code is performed on cpu which should die.
And simple calling zynq_slcr_cpu_stop() on cpu which should die just doesn't work.
There is probably any expectation which I can't see.

Quoted 3 lines

Have you checked whether the CPU to die can write to this register to stop its own clock and reset itself? Maybe you need a wfi() after writing to that register to trigger the hardware to shutdown the CPU?

One thing is clear here. Definitely cpu can't reset itself because it must be done in 3 steps and after the first cpu is in reset and without CLK. It means another cpu has to get another cpu out of reset.

Based on my tests cpu can't stop clock and reset itself.

Quoted 4 lines

It's also entirely possible that you need some other CPU to do those steps while the CPU to die spins in a loop or a wfi or something like that. You can hook into that via the cpu_kill() callback, which will have the CPU which is to die as its argument.

That's how it is written in the code right now.
Cpu 1, flush L1 cache and run that code in zynq_cpu_enter_lowpower code and then end up in wfi().
Then cpu0 runs cpu_kill function with argument 1 which means kill cpu1.

If cpu gets wakeup between wfi/cpu_kill is called then spurious is increase.

Quoted 4 lines

Without knowing how your hardware works, it's very difficult to be able to positively point you in an appropriate direction, but the fact that it's allegedly possible to turn the clock off and place the secondary CPU in reset means that this should be possible somehow.

I hope I have described zynq hw to get all information.

Because what seems to me problematic is.
1. In current code cpu1 is all the time in "for" loop and zynq_cpu_leave_lowpower is completely unused because there is no way to jump there
2. I expect that smp.c/cpu_die() is designed for system which should after wake-up jump to secondary_start_kernel to boot it. And this wakeup is performed from cpu0 from boot_secondary code.

Let me propose change for zynq which is like this.

void zynq_platform_cpu_die(unsigned int cpu)

{
	zynq_cpu_enter_lowpower();
	for (;;)
		cpu_do_idle();
}

Remove zynq_cpu_leave_lowpower, zynq_platform_do_lowpower + spurious var.

It never returns from zynq_platform_cpu_die function.
It should enter to lowpower before wfi(if this make sense)
to save energy because it can take some time till cpu0 calls kill function.

Is this a sensible solution?

Thanks,
Michal

Signature · 14 lines

Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130605/512c2c67/attachment.sig>

A message that is not in the archive (sent off-list or not yet received): OFF951DD26.F13B5CB1-ON48257B81.003EE068-48257B81.003F07FB@spreadtrum.com

Michal Simek

2013-06-05 12:07 UTC #17 lore raw

PATCH 1/1 ARM: zynq: wfi exit on same cpu is valid

Quoted 101 lines · Steve.Zhan at spreadtrum.com

linaro-kernel-bounces at lists.linaro.org wrote on 2013-06-05 18:47:46:
Quoted 11 lines
From: Michal Simek <[email protected]>
To: Russell King - ARM Linux <[email protected]>,
Cc: Lists linaro-kernel <[email protected]>, Patch Tracking <[email protected]>, michal.simek at xilinx.com, Lists LAKML <[email protected]>
Date: 2013-06-05 18:48
Subject: Re: [PATCH] ARM: zynq: wfi exit on same cpu is valid Sent by: linaro-kernel-bounces at lists.linaro.org
Quoted 1 line · Russell King - ARM Linux
On Tue, Jun 04, 2013 at 03:10:17PM +0100, Russell King - ARM Linux
Quoted 3 lines · wrote:
Quoted 3 lines
Quoted 3 lines · Daniel Lezcano
Quoted 2 lines · Amit Kucheria
Quoted 1 line
I'm curious why it is called pen_release. :) Is there some
historical
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
link to some HW lines?
I tried to figure out the same but I did not found any information
on
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
that. I assumed the name could be referring to a simplified mutual exclusion algorithm from the 'Dining philosophers problem' [1] where
the
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
fork is a pen.
Where it comes from is the original ARM SMP patches from early 2000,
which
Quoted 1 line
Quoted 1 line
Quoted 1 line
everyone has blindly copied with no thought about what they're doing.
This
Quoted 1 line
Quoted 1 line
Quoted 1 line
is why I'm totally against any consolidation of this code, because
I'm of
Quoted 1 line
Quoted 1 line
Quoted 1 line
the opinion that _no one_ other than the ARM Ltd development
platforms
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
should be using it.
"pen" means "holding pen". It comes about because early on in the
SMP
Quoted 1 line
Quoted 1 line
Quoted 1 line
development, ARM SMP platforms had four CPUs, and it was only
possible to
Quoted 1 line
Quoted 1 line
Quoted 1 line
release all three secondary CPUs from the boot loader simultaneously
to
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
a common piece of code.
As the kernel was not able to serialize the release of each CPU, ARM
Ltd
Quoted 1 line
Quoted 1 line
Quoted 1 line
worked around this problem by having all the CPUs jump to assembly
code
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
which "holds" the CPUs which we didn't want to boot yet, and the CPUs are released one at a time by setting pen_release to the hardware CPU number.
Modern platforms either have just one secondary CPU, or they have a
way
Quoted 1 line
Quoted 1 line
Quoted 1 line
to control the reset/power to the secondary CPU. This makes the
holding
Quoted 1 line
Quoted 1 line
Quoted 1 line
pen entirely redundant, and such platforms should _not_ make use of
any
Quoted 13 lines
Quoted 13 lines
Quoted 1 line
kind of holding pen.
And yes, indeed, zynq can control the secondary CPU:
void zynq_slcr_cpu_start(int cpu)
{
        /* enable CPUn */
        writel(SLCR_A9_CPU_CLKSTOP << cpu,
               zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
        /* enable CLK for CPUn */
        writel(0x0 << cpu, zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
}
Quoted 8 lines
Quoted 8 lines
void zynq_slcr_cpu_stop(int cpu)
{
        /* stop CLK and reset CPUn */
        writel((SLCR_A9_CPU_CLKSTOP | SLCR_A9_CPU_RST) << cpu,
               zynq_slcr_base + SLCR_A9_CPU_RST_CTRL);
}
So there's no need for the pen. There's no need for the low power
crap
Quoted 1 line
Quoted 1 line
in hotplug.c, there's no need for the pen in hotplug.c. You just
arrange
Quoted 12 lines
Quoted 5 lines
for the secondary CPU to have its clock stopped and reset when it is taken offline.
Hotplugging a CPU back in _should_ be no different from its initial bringup into the kernel.
I have tested that and cpu_die code is performed on cpu which should die.
And simple calling zynq_slcr_cpu_stop() on cpu which should die just doesn't work.
There is probably any expectation which I can't see.
Quoted 5 lines
Feel free to suggest me proper solution.
Thanks,
Michal

Quoted 8 lines

Hi Michal,
Because most SOC design is that secondary cpu can not poweecho 0 > /sys/devices/system/cpu/cpu1/onlinerdown by
itself.
These are some instructions is running in the bus.
We can only put the core in lowpower mode using wfi/wfe by itself.
If wakeup or boot this core from die/deepsleep again, we don't need to use holding pen.

pen usage is quite clear right now.

Let me comment Russel's comment.

Thanks,
Michal

Signature · 14 lines

Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130605/1bff5b6c/attachment.sig>