| Message ID | 1369997066-10585-1-git-send-email-sanjay.rawat@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/31/2013 12:44 PM, Sanjay Singh Rawat wrote: > 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 <sanjay.rawat@linaro.org> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > arch/arm/mach-zynq/hotplug.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/arch/arm/mach-zynq/hotplug.c b/arch/arm/mach-zynq/hotplug.c > index c89672b..a1ab22c 100644 > --- a/arch/arm/mach-zynq/hotplug.c > +++ b/arch/arm/mach-zynq/hotplug.c > @@ -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 >
Hi, On 06/03/2013 10:14 AM, Daniel Lezcano wrote: > On 05/31/2013 12:44 PM, Sanjay Singh Rawat wrote: >> 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 <sanjay.rawat@linaro.org> > > Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > >> --- >> arch/arm/mach-zynq/hotplug.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/arch/arm/mach-zynq/hotplug.c b/arch/arm/mach-zynq/hotplug.c >> index c89672b..a1ab22c 100644 >> --- a/arch/arm/mach-zynq/hotplug.c >> +++ b/arch/arm/mach-zynq/hotplug.c >> @@ -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; >> + } >> + 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
On 06/03/2013 11:51 AM, Michal Simek wrote: > Hi, > > On 06/03/2013 10:14 AM, Daniel Lezcano wrote: >> On 05/31/2013 12:44 PM, Sanjay Singh Rawat wrote: >>> 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 <sanjay.rawat@linaro.org> >> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> >> >>> --- >>> arch/arm/mach-zynq/hotplug.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/arch/arm/mach-zynq/hotplug.c b/arch/arm/mach-zynq/hotplug.c >>> index c89672b..a1ab22c 100644 >>> --- a/arch/arm/mach-zynq/hotplug.c >>> +++ b/arch/arm/mach-zynq/hotplug.c >>> @@ -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; >>> + } >>> + > 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 :)
On 06/03/2013 02:43 PM, Daniel Lezcano wrote: > On 06/03/2013 11:51 AM, Michal Simek wrote: >> Hi, >> >> On 06/03/2013 10:14 AM, Daniel Lezcano wrote: >>> On 05/31/2013 12:44 PM, Sanjay Singh Rawat wrote: >>>> 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 <sanjay.rawat@linaro.org> >>> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>> >>> >>>> --- >>>> arch/arm/mach-zynq/hotplug.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/arch/arm/mach-zynq/hotplug.c b/arch/arm/mach-zynq/hotplug.c >>>> index c89672b..a1ab22c 100644 >>>> --- a/arch/arm/mach-zynq/hotplug.c >>>> +++ b/arch/arm/mach-zynq/hotplug.c >>>> @@ -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; >>>> + } >>>> + >> 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 Thanks for explanation. I have added this patch to zynq/cleanup branch. > -- Daniel > > ps : sorry for my bad English :) No reason to apologise. Thanks, Michal
On Mon, Jun 3, 2013 at 6:13 PM, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > On 06/03/2013 11:51 AM, Michal Simek wrote: >> Hi, >> >> On 06/03/2013 10:14 AM, Daniel Lezcano wrote: >>> On 05/31/2013 12:44 PM, Sanjay Singh Rawat wrote: >>>> 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 <sanjay.rawat@linaro.org> >>> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>> >>> >>>> --- >>>> arch/arm/mach-zynq/hotplug.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/arch/arm/mach-zynq/hotplug.c b/arch/arm/mach-zynq/hotplug.c >>>> index c89672b..a1ab22c 100644 >>>> --- a/arch/arm/mach-zynq/hotplug.c >>>> +++ b/arch/arm/mach-zynq/hotplug.c >>>> @@ -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; >>>> + } >>>> + >> 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. I'm curious why it is called pen_release. :) Is there some historical link to some HW lines? > When the processors boot, all the processors except the CPU0 are in WFI.
On 06/04/2013 01:39 PM, Amit Kucheria wrote: > On Mon, Jun 3, 2013 at 6:13 PM, Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> On 06/03/2013 11:51 AM, Michal Simek wrote: >>> Hi, >>> >>> On 06/03/2013 10:14 AM, Daniel Lezcano wrote: >>>> On 05/31/2013 12:44 PM, Sanjay Singh Rawat wrote: >>>>> 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 <sanjay.rawat@linaro.org> >>>> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>> >>>> >>>>> --- >>>>> arch/arm/mach-zynq/hotplug.c | 7 +++++++ >>>>> 1 file changed, 7 insertions(+) >>>>> >>>>> diff --git a/arch/arm/mach-zynq/hotplug.c b/arch/arm/mach-zynq/hotplug.c >>>>> index c89672b..a1ab22c 100644 >>>>> --- a/arch/arm/mach-zynq/hotplug.c >>>>> +++ b/arch/arm/mach-zynq/hotplug.c >>>>> @@ -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; >>>>> + } >>>>> + >>> 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. > > 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
On 06/04/2013 09:29 AM, Michal Simek wrote: > On 06/03/2013 02:43 PM, Daniel Lezcano wrote: >> On 06/03/2013 11:51 AM, Michal Simek wrote: >>> Hi, >>> >>> On 06/03/2013 10:14 AM, Daniel Lezcano wrote: >>>> On 05/31/2013 12:44 PM, Sanjay Singh Rawat wrote: >>>>> 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 <sanjay.rawat@linaro.org> >>>> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>> >>>> >>>>> --- >>>>> arch/arm/mach-zynq/hotplug.c | 7 +++++++ >>>>> 1 file changed, 7 insertions(+) >>>>> >>>>> diff --git a/arch/arm/mach-zynq/hotplug.c b/arch/arm/mach-zynq/hotplug.c >>>>> index c89672b..a1ab22c 100644 >>>>> --- a/arch/arm/mach-zynq/hotplug.c >>>>> +++ b/arch/arm/mach-zynq/hotplug.c >>>>> @@ -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; >>>>> + } >>>>> + >>> 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 > > 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
On 06/04/2013 02:40 PM, Daniel Lezcano wrote: > On 06/04/2013 09:29 AM, Michal Simek wrote: >> On 06/03/2013 02:43 PM, Daniel Lezcano wrote: >>> On 06/03/2013 11:51 AM, Michal Simek wrote: >>>> Hi, >>>> >>>> On 06/03/2013 10:14 AM, Daniel Lezcano wrote: >>>>> On 05/31/2013 12:44 PM, Sanjay Singh Rawat wrote: >>>>>> 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 <sanjay.rawat@linaro.org> >>>>> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>>> >>>>> >>>>>> --- >>>>>> arch/arm/mach-zynq/hotplug.c | 7 +++++++ >>>>>> 1 file changed, 7 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm/mach-zynq/hotplug.c b/arch/arm/mach-zynq/hotplug.c >>>>>> index c89672b..a1ab22c 100644 >>>>>> --- a/arch/arm/mach-zynq/hotplug.c >>>>>> +++ b/arch/arm/mach-zynq/hotplug.c >>>>>> @@ -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; >>>>>> + } >>>>>> + >>>> 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 >> >> 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 > 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
On 06/04/2013 03:09 PM, Michal Simek wrote: > On 06/04/2013 02:40 PM, Daniel Lezcano wrote: >> On 06/04/2013 09:29 AM, Michal Simek wrote: >>> On 06/03/2013 02:43 PM, Daniel Lezcano wrote: >>>> On 06/03/2013 11:51 AM, Michal Simek wrote: >>>>> Hi, >>>>> >>>>> On 06/03/2013 10:14 AM, Daniel Lezcano wrote: >>>>>> On 05/31/2013 12:44 PM, Sanjay Singh Rawat wrote: >>>>>>> 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 <sanjay.rawat@linaro.org> >>>>>> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>>>> >>>>>> >>>>>>> --- >>>>>>> arch/arm/mach-zynq/hotplug.c | 7 +++++++ >>>>>>> 1 file changed, 7 insertions(+) >>>>>>> >>>>>>> diff --git a/arch/arm/mach-zynq/hotplug.c b/arch/arm/mach-zynq/hotplug.c >>>>>>> index c89672b..a1ab22c 100644 >>>>>>> --- a/arch/arm/mach-zynq/hotplug.c >>>>>>> +++ b/arch/arm/mach-zynq/hotplug.c >>>>>>> @@ -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; >>>>>>> + } >>>>>>> + >>>>> 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 >>> 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 > >> 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? When the cpu is booted, it runs the idle thread with its idle loop which calls the cpu_do_idle (WFI).
On Tue, Jun 04, 2013 at 01:58:31PM +0200, Daniel Lezcano wrote: > On 06/04/2013 01:39 PM, Amit Kucheria wrote: > > 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.
On Tue, Jun 04, 2013 at 03:10:17PM +0100, Russell King - ARM Linux wrote: > On Tue, Jun 04, 2013 at 01:58:31PM +0200, Daniel Lezcano wrote: > > On 06/04/2013 01:39 PM, Amit Kucheria wrote: > > > 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. 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.
On 06/04/2013 04:17 PM, Russell King - ARM Linux wrote: > On Tue, Jun 04, 2013 at 03:10:17PM +0100, Russell King - ARM Linux wrote: >> On Tue, Jun 04, 2013 at 01:58:31PM +0200, Daniel Lezcano wrote: >>> On 06/04/2013 01:39 PM, Amit Kucheria wrote: >>>> 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. > > 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. 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). > 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
On 06/04/2013 04:17 PM, Russell King - ARM Linux wrote: > On Tue, Jun 04, 2013 at 03:10:17PM +0100, Russell King - ARM Linux wrote: >> On Tue, Jun 04, 2013 at 01:58:31PM +0200, Daniel Lezcano wrote: >>> On 06/04/2013 01:39 PM, Amit Kucheria wrote: >>>> 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. > > 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. 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
On Wed, Jun 05, 2013 at 12:47:46PM +0200, Michal Simek wrote: > On 06/04/2013 04:17 PM, Russell King - ARM Linux wrote: > > 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. > > 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.
On 06/05/2013 01:28 PM, Steve.Zhan@spreadtrum.com wrote: > linaro-kernel-bounces@lists.linaro.org wrote on 2013-06-05 18:47:46: > >> From: Michal Simek <monstr@monstr.eu> >> To: Russell King - ARM Linux <linux@arm.linux.org.uk>, >> Cc: Lists linaro-kernel <linaro-kernel@lists.linaro.org>, Patch >> Tracking <patches@linaro.org>, michal.simek@xilinx.com, Lists LAKML >> <linux-arm-kernel@lists.infradead.org> >> Date: 2013-06-05 18:48 >> Subject: Re: [PATCH] ARM: zynq: wfi exit on same cpu is valid >> Sent by: linaro-kernel-bounces@lists.linaro.org >> >> On 06/04/2013 04:17 PM, Russell King - ARM Linux wrote: >>> On Tue, Jun 04, 2013 at 03:10:17PM +0100, Russell King - ARM Linux > wrote: >>>> On Tue, Jun 04, 2013 at 01:58:31PM +0200, Daniel Lezcano wrote: >>>>> On 06/04/2013 01:39 PM, Amit Kucheria wrote: >>>>>> 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. >>> >>> 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. >> >> 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 >> > > 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
On 06/05/2013 01:29 PM, Russell King - ARM Linux wrote: > On Wed, Jun 05, 2013 at 12:47:46PM +0200, Michal Simek wrote: >> On 06/04/2013 04:17 PM, Russell King - ARM Linux wrote: >>> 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. >> >> 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? 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. > 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. > 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
diff --git a/arch/arm/mach-zynq/hotplug.c b/arch/arm/mach-zynq/hotplug.c index c89672b..a1ab22c 100644 --- a/arch/arm/mach-zynq/hotplug.c +++ b/arch/arm/mach-zynq/hotplug.c @@ -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
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 <sanjay.rawat@linaro.org> --- arch/arm/mach-zynq/hotplug.c | 7 +++++++ 1 file changed, 7 insertions(+)