[PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Doug Anderson]

On exynos mcpm systems the firmware is hardcoded to jump to an address
in SRAM (0x02073000) when secondary CPUs come up.  By default the
firmware puts a bunch of code at that location.  That code expects the
kernel to fill in a few slots with addresses that it uses to jump back
to the kernel's entry point for secondary CPUs.

Originally (on prerelease hardware) this firmware code contained a
bunch of workarounds to deal with boot ROM bugs.  However on all
shipped hardware we simply use this code to redirect to a kernel
function for bringing up the CPUs.

Let's stop relying on the code provided by the bootloader and just
plumb in our own (simple) code jump to the kernel.  This has the nice
benefit of fixing problems due to the fact that older bootloaders
(like the one shipped on the Samsung Chromebook 2) might have put
slightly different code into this location.

Once suspend/resume is implemented for systems using exynos-mcpm we'll
need to make sure we reinstall our fixed up code after resume.  ...but
that's not anything new since IRAM (and thus the address of the
mcpm_entry_point) is lost across suspend/resume anyway.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 arch/arm/mach-exynos/mcpm-exynos.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
index 0498d0b..3a7fad0 100644
--- a/arch/arm/mach-exynos/mcpm-exynos.c
+++ b/arch/arm/mach-exynos/mcpm-exynos.c
@@ -343,11 +343,13 @@ static int __init exynos_mcpm_init(void)
 	pr_info("Exynos MCPM support installed\n");
 
 	/*
-	 * Future entries into the kernel can now go
-	 * through the cluster entry vectors.
+	 * U-Boot SPL is hardcoded to jump to the start of ns_sram_base_addr
+	 * as part of secondary_cpu_start().  Let's redirect it to the
+	 * mcpm_entry_point().
 	 */
-	__raw_writel(virt_to_phys(mcpm_entry_point),
-			ns_sram_base_addr + MCPM_BOOT_ADDR_OFFSET);
+	__raw_writel(0xe59f0000, ns_sram_base_addr);     /* ldr r0, [pc, #0] */
+	__raw_writel(0xe12fff10, ns_sram_base_addr + 4); /* bx  r0 */
+	__raw_writel(virt_to_phys(mcpm_entry_point), ns_sram_base_addr + 8);
 
 	iounmap(ns_sram_base_addr);
 
-- 
2.0.0.526.g5318336

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Nicolas Pitre]

On Fri, 6 Jun 2014, Doug Anderson wrote:

> On exynos mcpm systems the firmware is hardcoded to jump to an address
> in SRAM (0x02073000) when secondary CPUs come up.  By default the
> firmware puts a bunch of code at that location.  That code expects the
> kernel to fill in a few slots with addresses that it uses to jump back
> to the kernel's entry point for secondary CPUs.
> 
> Originally (on prerelease hardware) this firmware code contained a
> bunch of workarounds to deal with boot ROM bugs.  However on all
> shipped hardware we simply use this code to redirect to a kernel
> function for bringing up the CPUs.
> 
> Let's stop relying on the code provided by the bootloader and just
> plumb in our own (simple) code jump to the kernel.  This has the nice
> benefit of fixing problems due to the fact that older bootloaders
> (like the one shipped on the Samsung Chromebook 2) might have put
> slightly different code into this location.
> 
> Once suspend/resume is implemented for systems using exynos-mcpm we'll
> need to make sure we reinstall our fixed up code after resume.  ...but
> that's not anything new since IRAM (and thus the address of the
> mcpm_entry_point) is lost across suspend/resume anyway.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>  arch/arm/mach-exynos/mcpm-exynos.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
> index 0498d0b..3a7fad0 100644
> --- a/arch/arm/mach-exynos/mcpm-exynos.c
> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
> @@ -343,11 +343,13 @@ static int __init exynos_mcpm_init(void)
>  	pr_info("Exynos MCPM support installed\n");
>  
>  	/*
> -	 * Future entries into the kernel can now go
> -	 * through the cluster entry vectors.
> +	 * U-Boot SPL is hardcoded to jump to the start of ns_sram_base_addr
> +	 * as part of secondary_cpu_start().  Let's redirect it to the
> +	 * mcpm_entry_point().
>  	 */
> -	__raw_writel(virt_to_phys(mcpm_entry_point),
> -			ns_sram_base_addr + MCPM_BOOT_ADDR_OFFSET);
> +	__raw_writel(0xe59f0000, ns_sram_base_addr);     /* ldr r0, [pc, #0] */
> +	__raw_writel(0xe12fff10, ns_sram_base_addr + 4); /* bx  r0 */
> +	__raw_writel(virt_to_phys(mcpm_entry_point), ns_sram_base_addr + 8);

Is this valid for all exynos systems, or is this particular to
Chromebooks?

If this is all that is needed to solve the problem being discussed in 
the other thread I have absolutely no issue with such a workaround going 
into mainline.


Nicolas

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Doug Anderson]

Nicolas,

On Fri, Jun 6, 2014 at 3:35 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Fri, 6 Jun 2014, Doug Anderson wrote:
>
>> On exynos mcpm systems the firmware is hardcoded to jump to an address
>> in SRAM (0x02073000) when secondary CPUs come up.  By default the
>> firmware puts a bunch of code at that location.  That code expects the
>> kernel to fill in a few slots with addresses that it uses to jump back
>> to the kernel's entry point for secondary CPUs.
>>
>> Originally (on prerelease hardware) this firmware code contained a
>> bunch of workarounds to deal with boot ROM bugs.  However on all
>> shipped hardware we simply use this code to redirect to a kernel
>> function for bringing up the CPUs.
>>
>> Let's stop relying on the code provided by the bootloader and just
>> plumb in our own (simple) code jump to the kernel.  This has the nice
>> benefit of fixing problems due to the fact that older bootloaders
>> (like the one shipped on the Samsung Chromebook 2) might have put
>> slightly different code into this location.
>>
>> Once suspend/resume is implemented for systems using exynos-mcpm we'll
>> need to make sure we reinstall our fixed up code after resume.  ...but
>> that's not anything new since IRAM (and thus the address of the
>> mcpm_entry_point) is lost across suspend/resume anyway.
>>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>> ---
>>  arch/arm/mach-exynos/mcpm-exynos.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
>> index 0498d0b..3a7fad0 100644
>> --- a/arch/arm/mach-exynos/mcpm-exynos.c
>> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
>> @@ -343,11 +343,13 @@ static int __init exynos_mcpm_init(void)
>>       pr_info("Exynos MCPM support installed\n");
>>
>>       /*
>> -      * Future entries into the kernel can now go
>> -      * through the cluster entry vectors.
>> +      * U-Boot SPL is hardcoded to jump to the start of ns_sram_base_addr
>> +      * as part of secondary_cpu_start().  Let's redirect it to the
>> +      * mcpm_entry_point().
>>        */
>> -     __raw_writel(virt_to_phys(mcpm_entry_point),
>> -                     ns_sram_base_addr + MCPM_BOOT_ADDR_OFFSET);
>> +     __raw_writel(0xe59f0000, ns_sram_base_addr);     /* ldr r0, [pc, #0] */
>> +     __raw_writel(0xe12fff10, ns_sram_base_addr + 4); /* bx  r0 */
>> +     __raw_writel(virt_to_phys(mcpm_entry_point), ns_sram_base_addr + 8);
>
> Is this valid for all exynos systems, or is this particular to
> Chromebooks?

I will have to let others comment on that since I can't be certain of
anything other than the firmware branch I've seen.

...but given that the code before my patch was putting the resume
address at the magic 0x0207301C and that matches the code I've seen,
I'm going to say that they all behave the same.


> If this is all that is needed to solve the problem being discussed in
> the other thread I have absolutely no issue with such a workaround going
> into mainline.

This plus the CCI fix that Andrew is planning to post.

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Andrew Bresticker]

>> If this is all that is needed to solve the problem being discussed in
>> the other thread I have absolutely no issue with such a workaround going
>> into mainline.
>
> This plus the CCI fix that Andrew is planning to post.

Right - we'll need a patch to enable the CCI port for the cluster we
booted on at boot and resume time since the first CPU up won't enter
through exynos_pm_power_up_setup().  I'll try to post something later
today or Monday.

[PATCH v2] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Doug Anderson]

On exynos mcpm systems the firmware is hardcoded to jump to an address
in SRAM (0x02073000) when secondary CPUs come up.  By default the
firmware puts a bunch of code at that location.  That code expects the
kernel to fill in a few slots with addresses that it uses to jump back
to the kernel's entry point for secondary CPUs.

Originally (on prerelease hardware) this firmware code contained a
bunch of workarounds to deal with boot ROM bugs.  However on all
shipped hardware we simply use this code to redirect to a kernel
function for bringing up the CPUs.

Let's stop relying on the code provided by the bootloader and just
plumb in our own (simple) code jump to the kernel.  This has the nice
benefit of fixing problems due to the fact that older bootloaders
(like the one shipped on the Samsung Chromebook 2) might have put
slightly different code into this location.

Once suspend/resume is implemented for systems using exynos-mcpm we'll
need to make sure we reinstall our fixed up code after resume.  ...but
that's not anything new since IRAM (and thus the address of the
mcpm_entry_point) is lost across suspend/resume anyway.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v2:
- Removed #define

 arch/arm/mach-exynos/mcpm-exynos.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
index 0498d0b..ace0ed6 100644
--- a/arch/arm/mach-exynos/mcpm-exynos.c
+++ b/arch/arm/mach-exynos/mcpm-exynos.c
@@ -25,7 +25,6 @@
 
 #define EXYNOS5420_CPUS_PER_CLUSTER	4
 #define EXYNOS5420_NR_CLUSTERS		2
-#define MCPM_BOOT_ADDR_OFFSET		0x1c
 
 /*
  * The common v7_exit_coherency_flush API could not be used because of the
@@ -343,11 +342,13 @@ static int __init exynos_mcpm_init(void)
 	pr_info("Exynos MCPM support installed\n");
 
 	/*
-	 * Future entries into the kernel can now go
-	 * through the cluster entry vectors.
+	 * U-Boot SPL is hardcoded to jump to the start of ns_sram_base_addr
+	 * as part of secondary_cpu_start().  Let's redirect it to the
+	 * mcpm_entry_point().
 	 */
-	__raw_writel(virt_to_phys(mcpm_entry_point),
-			ns_sram_base_addr + MCPM_BOOT_ADDR_OFFSET);
+	__raw_writel(0xe59f0000, ns_sram_base_addr);     /* ldr r0, [pc, #0] */
+	__raw_writel(0xe12fff10, ns_sram_base_addr + 4); /* bx  r0 */
+	__raw_writel(virt_to_phys(mcpm_entry_point), ns_sram_base_addr + 8);
 
 	iounmap(ns_sram_base_addr);
 
-- 
2.0.0.526.g5318336

Re: [PATCH v2] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Nicolas Pitre]

On Fri, 6 Jun 2014, Doug Anderson wrote:

> On exynos mcpm systems the firmware is hardcoded to jump to an address
> in SRAM (0x02073000) when secondary CPUs come up.  By default the
> firmware puts a bunch of code at that location.  That code expects the
> kernel to fill in a few slots with addresses that it uses to jump back
> to the kernel's entry point for secondary CPUs.
> 
> Originally (on prerelease hardware) this firmware code contained a
> bunch of workarounds to deal with boot ROM bugs.  However on all
> shipped hardware we simply use this code to redirect to a kernel
> function for bringing up the CPUs.
> 
> Let's stop relying on the code provided by the bootloader and just
> plumb in our own (simple) code jump to the kernel.  This has the nice
> benefit of fixing problems due to the fact that older bootloaders
> (like the one shipped on the Samsung Chromebook 2) might have put
> slightly different code into this location.
> 
> Once suspend/resume is implemented for systems using exynos-mcpm we'll
> need to make sure we reinstall our fixed up code after resume.  ...but
> that's not anything new since IRAM (and thus the address of the
> mcpm_entry_point) is lost across suspend/resume anyway.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>

Acked-by: Nicolas Pitre <nico@linaro.org>

> ---
> Changes in v2:
> - Removed #define
> 
>  arch/arm/mach-exynos/mcpm-exynos.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
> index 0498d0b..ace0ed6 100644
> --- a/arch/arm/mach-exynos/mcpm-exynos.c
> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
> @@ -25,7 +25,6 @@
>  
>  #define EXYNOS5420_CPUS_PER_CLUSTER	4
>  #define EXYNOS5420_NR_CLUSTERS		2
> -#define MCPM_BOOT_ADDR_OFFSET		0x1c
>  
>  /*
>   * The common v7_exit_coherency_flush API could not be used because of the
> @@ -343,11 +342,13 @@ static int __init exynos_mcpm_init(void)
>  	pr_info("Exynos MCPM support installed\n");
>  
>  	/*
> -	 * Future entries into the kernel can now go
> -	 * through the cluster entry vectors.
> +	 * U-Boot SPL is hardcoded to jump to the start of ns_sram_base_addr
> +	 * as part of secondary_cpu_start().  Let's redirect it to the
> +	 * mcpm_entry_point().
>  	 */
> -	__raw_writel(virt_to_phys(mcpm_entry_point),
> -			ns_sram_base_addr + MCPM_BOOT_ADDR_OFFSET);
> +	__raw_writel(0xe59f0000, ns_sram_base_addr);     /* ldr r0, [pc, #0] */
> +	__raw_writel(0xe12fff10, ns_sram_base_addr + 4); /* bx  r0 */
> +	__raw_writel(virt_to_phys(mcpm_entry_point), ns_sram_base_addr + 8);
>  
>  	iounmap(ns_sram_base_addr);
>  
> -- 
> 2.0.0.526.g5318336
> 

Re: [PATCH v2] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Kevin Hilman]

Doug Anderson <dianders@chromium.org> writes:

> On exynos mcpm systems the firmware is hardcoded to jump to an address
> in SRAM (0x02073000) when secondary CPUs come up.  By default the
> firmware puts a bunch of code at that location.  That code expects the
> kernel to fill in a few slots with addresses that it uses to jump back
> to the kernel's entry point for secondary CPUs.
>
> Originally (on prerelease hardware) this firmware code contained a
> bunch of workarounds to deal with boot ROM bugs.  However on all
> shipped hardware we simply use this code to redirect to a kernel
> function for bringing up the CPUs.
>
> Let's stop relying on the code provided by the bootloader and just
> plumb in our own (simple) code jump to the kernel.  This has the nice
> benefit of fixing problems due to the fact that older bootloaders
> (like the one shipped on the Samsung Chromebook 2) might have put
> slightly different code into this location.
>
> Once suspend/resume is implemented for systems using exynos-mcpm we'll
> need to make sure we reinstall our fixed up code after resume.  ...but
> that's not anything new since IRAM (and thus the address of the
> mcpm_entry_point) is lost across suspend/resume anyway.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>

Acked-by: Kevin Hilman <khilman@linaro.org>
Tested-by: Kevin Hilman <khilman@linaro.org>

I confirm that this patch (plus the enable CCI hack[1]) allows me to see
all 8 cores when booting linux-next on my Chromebook2.

Kevin

[1] While waiting for the forth-coming patch from Andrew to enable the
    CCI port for the boot cluster), I do this from u-boot before starting
    the kernel (based on earlier email from Doug):

    mw.l 10d25000 3  # Enable CCI from U-Boot

Re: [PATCH v2] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Andrew Bresticker]

> [1] While waiting for the forth-coming patch from Andrew to enable the
>     CCI port for the boot cluster), I do this from u-boot before starting
>     the kernel (based on earlier email from Doug):
>
>     mw.l 10d25000 3  # Enable CCI from U-Boot

>From the other thread, it sounds like Nicolas wants enabling of the
boot cluster's CCI port to be done unconditionally for all MCPM
platforms.  Nicolas, are you preparing a patch for this or should I?
The only issue I see with making the MCPM loopback generic is that
although all current mainline MCPM platforms have the same cache flush
procedure, a future platform could be different.

Thanks,
Andrew

Re: [PATCH v2] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Nicolas Pitre]

On Mon, 9 Jun 2014, Andrew Bresticker wrote:

> > [1] While waiting for the forth-coming patch from Andrew to enable the
> >     CCI port for the boot cluster), I do this from u-boot before starting
> >     the kernel (based on earlier email from Doug):
> >
> >     mw.l 10d25000 3  # Enable CCI from U-Boot
> 
> From the other thread, it sounds like Nicolas wants enabling of the
> boot cluster's CCI port to be done unconditionally for all MCPM
> platforms.  Nicolas, are you preparing a patch for this or should I?

I would like confirmation this indeed solves your problem first before I 
go ahead with it.

> The only issue I see with making the MCPM loopback generic is that
> although all current mainline MCPM platforms have the same cache flush
> procedure, a future platform could be different.

If you look at my patch the cache flush is factored out.


Nicolas

Re: [PATCH v2] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Kevin Hilman]

On Mon, Jun 9, 2014 at 1:22 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Mon, 9 Jun 2014, Andrew Bresticker wrote:
>
>> > [1] While waiting for the forth-coming patch from Andrew to enable the
>> >     CCI port for the boot cluster), I do this from u-boot before starting
>> >     the kernel (based on earlier email from Doug):
>> >
>> >     mw.l 10d25000 3  # Enable CCI from U-Boot
>>
>> From the other thread, it sounds like Nicolas wants enabling of the
>> boot cluster's CCI port to be done unconditionally for all MCPM
>> platforms.  Nicolas, are you preparing a patch for this or should I?
>
> I would like confirmation this indeed solves your problem first before I
> go ahead with it.

FYI... I confirmed on the other thread.  It works on the Chromebook2.

Kevin

Re: [PATCH v2] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Nicolas Pitre]

On Mon, 9 Jun 2014, Kevin Hilman wrote:

> On Mon, Jun 9, 2014 at 1:22 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > On Mon, 9 Jun 2014, Andrew Bresticker wrote:
> >
> >> > [1] While waiting for the forth-coming patch from Andrew to enable the
> >> >     CCI port for the boot cluster), I do this from u-boot before starting
> >> >     the kernel (based on earlier email from Doug):
> >> >
> >> >     mw.l 10d25000 3  # Enable CCI from U-Boot
> >>
> >> From the other thread, it sounds like Nicolas wants enabling of the
> >> boot cluster's CCI port to be done unconditionally for all MCPM
> >> platforms.  Nicolas, are you preparing a patch for this or should I?
> >
> > I would like confirmation this indeed solves your problem first before I
> > go ahead with it.
> 
> FYI... I confirmed on the other thread.  It works on the Chromebook2.

OK I'll clean it up for mainline.


Nicolas

Re: [PATCH v2] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Doug Anderson]

Nicolas,

On Mon, Jun 9, 2014 at 1:55 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Mon, 9 Jun 2014, Kevin Hilman wrote:
>
>> On Mon, Jun 9, 2014 at 1:22 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
>> > On Mon, 9 Jun 2014, Andrew Bresticker wrote:
>> >
>> >> > [1] While waiting for the forth-coming patch from Andrew to enable the
>> >> >     CCI port for the boot cluster), I do this from u-boot before starting
>> >> >     the kernel (based on earlier email from Doug):
>> >> >
>> >> >     mw.l 10d25000 3  # Enable CCI from U-Boot
>> >>
>> >> From the other thread, it sounds like Nicolas wants enabling of the
>> >> boot cluster's CCI port to be done unconditionally for all MCPM
>> >> platforms.  Nicolas, are you preparing a patch for this or should I?
>> >
>> > I would like confirmation this indeed solves your problem first before I
>> > go ahead with it.
>>
>> FYI... I confirmed on the other thread.  It works on the Chromebook2.
>
> OK I'll clean it up for mainline.

I just wanted to double-check that this is still on your list of
things to do.  If not then please let us know!  :)

-Doug

Re: [PATCH v2] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Nicolas Pitre]

On Mon, 16 Jun 2014, Doug Anderson wrote:

> Nicolas,
> 
> On Mon, Jun 9, 2014 at 1:55 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > On Mon, 9 Jun 2014, Kevin Hilman wrote:
> >
> >> On Mon, Jun 9, 2014 at 1:22 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> >> > On Mon, 9 Jun 2014, Andrew Bresticker wrote:
> >> >
> >> >> > [1] While waiting for the forth-coming patch from Andrew to enable the
> >> >> >     CCI port for the boot cluster), I do this from u-boot before starting
> >> >> >     the kernel (based on earlier email from Doug):
> >> >> >
> >> >> >     mw.l 10d25000 3  # Enable CCI from U-Boot
> >> >>
> >> >> From the other thread, it sounds like Nicolas wants enabling of the
> >> >> boot cluster's CCI port to be done unconditionally for all MCPM
> >> >> platforms.  Nicolas, are you preparing a patch for this or should I?
> >> >
> >> > I would like confirmation this indeed solves your problem first before I
> >> > go ahead with it.
> >>
> >> FYI... I confirmed on the other thread.  It works on the Chromebook2.
> >
> > OK I'll clean it up for mainline.
> 
> I just wanted to double-check that this is still on your list of
> things to do.  If not then please let us know!  :)

It is.


Nicolas

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Lorenzo Pieralisi]

On Fri, Jun 06, 2014 at 10:43:05PM +0100, Doug Anderson wrote:
> On exynos mcpm systems the firmware is hardcoded to jump to an address
> in SRAM (0x02073000) when secondary CPUs come up.  By default the
> firmware puts a bunch of code at that location.  That code expects the
> kernel to fill in a few slots with addresses that it uses to jump back
> to the kernel's entry point for secondary CPUs.
> 
> Originally (on prerelease hardware) this firmware code contained a
> bunch of workarounds to deal with boot ROM bugs.  However on all
> shipped hardware we simply use this code to redirect to a kernel
> function for bringing up the CPUs.
> 
> Let's stop relying on the code provided by the bootloader and just
> plumb in our own (simple) code jump to the kernel.  This has the nice
> benefit of fixing problems due to the fact that older bootloaders
> (like the one shipped on the Samsung Chromebook 2) might have put
> slightly different code into this location.
> 
> Once suspend/resume is implemented for systems using exynos-mcpm we'll
> need to make sure we reinstall our fixed up code after resume.  ...but
> that's not anything new since IRAM (and thus the address of the
> mcpm_entry_point) is lost across suspend/resume anyway.

Can I ask you please what the firmware does for the boot (primary) cpu
on cold-boot and warm-boot (resume from suspend) ?

Does it jump to a specific (hardcoded) location ?

Is the primary CPU (MPIDR) hardcoded in firmware so that on both
cold and warm-boot firmware sees a specific MPIDR as "special" ?

I am asking to check if on this platform CPUidle (where the notion of
primary CPU disappears) has a chance to run properly.

Probably CPUidle won't attain idle states where IRAM content is lost, but I
am still worried about the primary vs secondaries firmware boot behaviour.

What happens on reboot from suspend to RAM (or to put it differently,
what does secure firmware do on reboot from suspend to RAM - in
particular how is the "jump" address to bootloader/kernel set ?)

Thank you very much.

Lorenzo

> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>  arch/arm/mach-exynos/mcpm-exynos.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
> index 0498d0b..3a7fad0 100644
> --- a/arch/arm/mach-exynos/mcpm-exynos.c
> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
> @@ -343,11 +343,13 @@ static int __init exynos_mcpm_init(void)
>  	pr_info("Exynos MCPM support installed\n");
>  
>  	/*
> -	 * Future entries into the kernel can now go
> -	 * through the cluster entry vectors.
> +	 * U-Boot SPL is hardcoded to jump to the start of ns_sram_base_addr
> +	 * as part of secondary_cpu_start().  Let's redirect it to the
> +	 * mcpm_entry_point().
>  	 */
> -	__raw_writel(virt_to_phys(mcpm_entry_point),
> -			ns_sram_base_addr + MCPM_BOOT_ADDR_OFFSET);
> +	__raw_writel(0xe59f0000, ns_sram_base_addr);     /* ldr r0, [pc, #0] */
> +	__raw_writel(0xe12fff10, ns_sram_base_addr + 4); /* bx  r0 */
> +	__raw_writel(virt_to_phys(mcpm_entry_point), ns_sram_base_addr + 8);
>  
>  	iounmap(ns_sram_base_addr);
>  
> -- 
> 2.0.0.526.g5318336
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Doug Anderson]

Lorenzo,

On Sat, Jun 7, 2014 at 11:12 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Fri, Jun 06, 2014 at 10:43:05PM +0100, Doug Anderson wrote:
>> On exynos mcpm systems the firmware is hardcoded to jump to an address
>> in SRAM (0x02073000) when secondary CPUs come up.  By default the
>> firmware puts a bunch of code at that location.  That code expects the
>> kernel to fill in a few slots with addresses that it uses to jump back
>> to the kernel's entry point for secondary CPUs.
>>
>> Originally (on prerelease hardware) this firmware code contained a
>> bunch of workarounds to deal with boot ROM bugs.  However on all
>> shipped hardware we simply use this code to redirect to a kernel
>> function for bringing up the CPUs.
>>
>> Let's stop relying on the code provided by the bootloader and just
>> plumb in our own (simple) code jump to the kernel.  This has the nice
>> benefit of fixing problems due to the fact that older bootloaders
>> (like the one shipped on the Samsung Chromebook 2) might have put
>> slightly different code into this location.
>>
>> Once suspend/resume is implemented for systems using exynos-mcpm we'll
>> need to make sure we reinstall our fixed up code after resume.  ...but
>> that's not anything new since IRAM (and thus the address of the
>> mcpm_entry_point) is lost across suspend/resume anyway.
>
> Can I ask you please what the firmware does for the boot (primary) cpu
> on cold-boot and warm-boot (resume from suspend) ?

On cold boot:

1. iROM loads BL1 from SPI flash
2. BL1 loads BL2 (SPL) from SPI flash
3. BL2 loads loads RO U-Boot from SPI flash
4. RO U-Boot (might) load RW U-Boot from SPI flash
5. U-Boot loads kernel from eMMC (or SD card or USB)
6. U-Boot boots kernel using bootm.


On resume from suspend:

1. iROM loads BL1 from SPI flash

2. BL1 loads BL2 (SPL) from SPI flash

3. BL2 does some basic init code (clocks, SDRAM out of self refresh,
...) and looks at some SoC registers that are preserved across
suspend/resume.  These registers contain flags indicating that this is
a resume from suspend and a pointer to the address in the kernel to
jump to at resume time.  The address of these registers and which bits
mean which flags is hardcoded and is part of the contract between the
bootloader and the kernel.

4. BL2 jumps to kernel.

---

I will note that BL1, BL2, and "RO U-Boot" are read only in production
systems.  Note that at resume time only Read-Only code is loaded from
persistent storage.  That means no extra verification is needed.  A
system exploit could survive suspend/resume but a reboot would clear
it and a reboot + suspend/resume wouldn't bring it back.


> Does it jump to a specific (hardcoded) location ?

I guess I could say it this way:

A) In resume from suspend, by agreement between the kernel and the
bootloader we'll jump to the address stored in 0x10040800 (INFORM0)

B) In bringing up a new CPU, by agreement between the kernel and the
bootloader we'll jump directly to 0x02073000 (and address in the SoC's
internal SRAM).


> Is the primary CPU (MPIDR) hardcoded in firmware so that on both
> cold and warm-boot firmware sees a specific MPIDR as "special" ?

Cold boot and resume from suspend are detected via various special
flags in various special locations.  Resume from suspend looks at
INFORM1 (0x10048004) for flags.  This register is 0 during a cold boot
and has special values set by the kernel at resume time.

It also looks as if some code looks at 0x10040900 (PMU_SPARE0) to help
tell initial cold boot and secondary CPU bringup.


> I am asking to check if on this platform CPUidle (where the notion of
> primary CPU disappears) has a chance to run properly.

I believe it should be possible, but we don't have CPUidle implemented
in our current system.  Abhilash may be able to comment more.


> Probably CPUidle won't attain idle states where IRAM content is lost, but I
> am still worried about the primary vs secondaries firmware boot behaviour.

I don't think iRAM can be turned off for CPUidle.


> What happens on reboot from suspend to RAM (or to put it differently,
> what does secure firmware do on reboot from suspend to RAM - in
> particular how is the "jump" address to bootloader/kernel set ?)

Should be described above now.

-Doug

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Lorenzo Pieralisi]

On Mon, Jun 09, 2014 at 06:03:31PM +0100, Doug Anderson wrote:

[...]

> Cold boot and resume from suspend are detected via various special
> flags in various special locations.  Resume from suspend looks at
> INFORM1 (0x10048004) for flags.  This register is 0 during a cold boot
> and has special values set by the kernel at resume time.
> 
> It also looks as if some code looks at 0x10040900 (PMU_SPARE0) to help
> tell initial cold boot and secondary CPU bringup.

Ok, thanks a lot. It looks like firmware paths should be ready to
detect cold vs warm boot, and hopefully do not rely on a specific
MPIDR to come up first out of power states.

> > I am asking to check if on this platform CPUidle (where the notion of
> > primary CPU disappears) has a chance to run properly.
> 
> I believe it should be possible, but we don't have CPUidle implemented
> in our current system.  Abhilash may be able to comment more.

I am interested in more insights, that's very helpful thanks.

> > Probably CPUidle won't attain idle states where IRAM content is lost, but I
> > am still worried about the primary vs secondaries firmware boot behaviour.
> 
> I don't think iRAM can be turned off for CPUidle.

It might be added a system state but I doubt that too and if you are
relying on registers for jump addresses that's not even a problem in
the first place.

> > What happens on reboot from suspend to RAM (or to put it differently,
> > what does secure firmware do on reboot from suspend to RAM - in
> > particular how is the "jump" address to bootloader/kernel set ?)
> 
> Should be described above now.

Thank you very much.

Lorenzo

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Chander Kashyap]

On 10 June 2014 04:08, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Mon, Jun 09, 2014 at 06:03:31PM +0100, Doug Anderson wrote:
>
> [...]
>
>> Cold boot and resume from suspend are detected via various special
>> flags in various special locations.  Resume from suspend looks at
>> INFORM1 (0x10048004) for flags.  This register is 0 during a cold boot
>> and has special values set by the kernel at resume time.
>>
>> It also looks as if some code looks at 0x10040900 (PMU_SPARE0) to help
>> tell initial cold boot and secondary CPU bringup.
>
> Ok, thanks a lot. It looks like firmware paths should be ready to
> detect cold vs warm boot, and hopefully do not rely on a specific
> MPIDR to come up first out of power states.
>
>> > I am asking to check if on this platform CPUidle (where the notion of
>> > primary CPU disappears) has a chance to run properly.
>>
>> I believe it should be possible, but we don't have CPUidle implemented
>> in our current system.  Abhilash may be able to comment more.
>

Cpuidle is implemented for exynos5420, and is tested on chromebook.


> I am interested in more insights, that's very helpful thanks.
>
>> > Probably CPUidle won't attain idle states where IRAM content is lost, but I
>> > am still worried about the primary vs secondaries firmware boot behaviour.
>>
>> I don't think iRAM can be turned off for CPUidle.

Yes thats true.
>
> It might be added a system state but I doubt that too and if you are
> relying on registers for jump addresses that's not even a problem in
> the first place.
>
>> > What happens on reboot from suspend to RAM (or to put it differently,
>> > what does secure firmware do on reboot from suspend to RAM - in
>> > particular how is the "jump" address to bootloader/kernel set ?)
>>
>> Should be described above now.
>
> Thank you very much.
>
> Lorenzo
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
with warm regards,
Chander Kashyap

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Doug Anderson]

Chander,

On Tue, Jun 10, 2014 at 1:12 AM, Chander Kashyap
<chander.kashyap@linaro.org> wrote:
> On 10 June 2014 04:08, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>> On Mon, Jun 09, 2014 at 06:03:31PM +0100, Doug Anderson wrote:
>>
>> [...]
>>
>>> Cold boot and resume from suspend are detected via various special
>>> flags in various special locations.  Resume from suspend looks at
>>> INFORM1 (0x10048004) for flags.  This register is 0 during a cold boot
>>> and has special values set by the kernel at resume time.
>>>
>>> It also looks as if some code looks at 0x10040900 (PMU_SPARE0) to help
>>> tell initial cold boot and secondary CPU bringup.
>>
>> Ok, thanks a lot. It looks like firmware paths should be ready to
>> detect cold vs warm boot, and hopefully do not rely on a specific
>> MPIDR to come up first out of power states.
>>
>>> > I am asking to check if on this platform CPUidle (where the notion of
>>> > primary CPU disappears) has a chance to run properly.
>>>
>>> I believe it should be possible, but we don't have CPUidle implemented
>>> in our current system.  Abhilash may be able to comment more.
>>
>
> Cpuidle is implemented for exynos5420, and is tested on chromebook.

My S-state knowledge is not strong, but I believe that Lorenzo's
questions matter if we're using S2 for CPUidle (where we actually turn
off power and hot unplug CPUs) but not when we're using S1 for CPUidle
(where we just enter WFI/WFE).

I believe that in ChromeOS we use S1 CPUidle and that it works fine.
We've never implemented S2 that I'm aware of.

-Doug

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Nicolas Pitre]

On Tue, 10 Jun 2014, Doug Anderson wrote:

> My S-state knowledge is not strong, but I believe that Lorenzo's
> questions matter if we're using S2 for CPUidle (where we actually turn
> off power and hot unplug CPUs) but not when we're using S1 for CPUidle
> (where we just enter WFI/WFE).
> 
> I believe that in ChromeOS we use S1 CPUidle and that it works fine.
> We've never implemented S2 that I'm aware of.

You'll have to rely on MCPM for that.  That's probably why it hasn't 
been implemented before.


Nicolas

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Chander Kashyap]

Hi Doug,

On Tue, Jun 10, 2014 at 9:19 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Tue, 10 Jun 2014, Doug Anderson wrote:
>
>> My S-state knowledge is not strong, but I believe that Lorenzo's
>> questions matter if we're using S2 for CPUidle (where we actually turn
>> off power and hot unplug CPUs) but not when we're using S1 for CPUidle
>> (where we just enter WFI/WFE).
>>

No Its not plain WFI.

All cores in Exynos5420 can be powered off independently.
This functionality has been tested.

Below is the link for the posted patches.

https://lkml.org/lkml/2014/6/10/194

And as Nicolas wrote, these patches need MCPM for that.

>> I believe that in ChromeOS we use S1 CPUidle and that it works fine.
>> We've never implemented S2 that I'm aware of.
>
> You'll have to rely on MCPM for that.  That's probably why it hasn't
> been implemented before.
>
>
> Nicolas
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Lorenzo Pieralisi]

On Wed, Jun 11, 2014 at 05:52:10AM +0100, Chander Kashyap wrote:
> Hi Doug,
> 
> On Tue, Jun 10, 2014 at 9:19 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > On Tue, 10 Jun 2014, Doug Anderson wrote:
> >
> >> My S-state knowledge is not strong, but I believe that Lorenzo's
> >> questions matter if we're using S2 for CPUidle (where we actually turn
> >> off power and hot unplug CPUs) but not when we're using S1 for CPUidle
> >> (where we just enter WFI/WFE).
> >>
> 
> No Its not plain WFI.
> 
> All cores in Exynos5420 can be powered off independently.
> This functionality has been tested.
> 
> Below is the link for the posted patches.
> 
> https://lkml.org/lkml/2014/6/10/194
> 
> And as Nicolas wrote, these patches need MCPM for that.

Chander, I cast a look into the code and I have a question
(you also told me on CPUidle review that only core off
is supported in CPUidle).

When you say all cores can be powered off independently, do
you also mean that clusters can be powered off (in CPUidle) ?

I am pointing this out since in the MCPM backend I noticed:

"/* TODO: Turn off the cluster here to save power. */"

I do not see any cluster power down request in the down path.

If I am wrong, ignore my message, I am just writing to help.

If you can only power down cores, but not the clusters on idle,
please keep in mind that you are currently cleaning and invalidating
the L2 when last man is running and this must not be taken
lightly since, if L2 stays on, that's a massive energy waste
for nothing.

So, if clusters stay up, you _have_ to tweak the MCPM backend slightly
to avoid cleaning L2, that's pivotal.

Lorenzo

> 
> >> I believe that in ChromeOS we use S1 CPUidle and that it works fine.
> >> We've never implemented S2 that I'm aware of.
> >
> > You'll have to rely on MCPM for that.  That's probably why it hasn't
> > been implemented before.
> >
> >
> > Nicolas
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Chander Kashyap]

On Wed, Jun 11, 2014 at 3:43 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Wed, Jun 11, 2014 at 05:52:10AM +0100, Chander Kashyap wrote:
>> Hi Doug,
>>
>> On Tue, Jun 10, 2014 at 9:19 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
>> > On Tue, 10 Jun 2014, Doug Anderson wrote:
>> >
>> >> My S-state knowledge is not strong, but I believe that Lorenzo's
>> >> questions matter if we're using S2 for CPUidle (where we actually turn
>> >> off power and hot unplug CPUs) but not when we're using S1 for CPUidle
>> >> (where we just enter WFI/WFE).
>> >>
>>
>> No Its not plain WFI.
>>
>> All cores in Exynos5420 can be powered off independently.
>> This functionality has been tested.
>>
>> Below is the link for the posted patches.
>>
>> https://lkml.org/lkml/2014/6/10/194
>>
>> And as Nicolas wrote, these patches need MCPM for that.
>
> Chander, I cast a look into the code and I have a question
> (you also told me on CPUidle review that only core off
> is supported in CPUidle).
>
> When you say all cores can be powered off independently, do
> you also mean that clusters can be powered off (in CPUidle) ?
>
> I am pointing this out since in the MCPM backend I noticed:
>
> "/* TODO: Turn off the cluster here to save power. */"
>
> I do not see any cluster power down request in the down path.
>
> If I am wrong, ignore my message, I am just writing to help.
>
> If you can only power down cores, but not the clusters on idle,
> please keep in mind that you are currently cleaning and invalidating
> the L2 when last man is running and this must not be taken
> lightly since, if L2 stays on, that's a massive energy waste
> for nothing.
>
> So, if clusters stay up, you _have_ to tweak the MCPM backend slightly
> to avoid cleaning L2, that's pivotal.

Hi Lorenzo,
Cluster shutdown is in progress. Abhilash will add support for that.

https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg31104.html


>
> Lorenzo
>
>>
>> >> I believe that in ChromeOS we use S1 CPUidle and that it works fine.
>> >> We've never implemented S2 that I'm aware of.
>> >
>> > You'll have to rely on MCPM for that.  That's probably why it hasn't
>> > been implemented before.
>> >
>> >
>> > Nicolas
>> >
>> > _______________________________________________
>> > linux-arm-kernel mailing list
>> > linux-arm-kernel@lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Lorenzo Pieralisi]

On Wed, Jun 11, 2014 at 01:14:21PM +0100, Chander Kashyap wrote:
> On Wed, Jun 11, 2014 at 3:43 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > On Wed, Jun 11, 2014 at 05:52:10AM +0100, Chander Kashyap wrote:
> >> Hi Doug,
> >>
> >> On Tue, Jun 10, 2014 at 9:19 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> >> > On Tue, 10 Jun 2014, Doug Anderson wrote:
> >> >
> >> >> My S-state knowledge is not strong, but I believe that Lorenzo's
> >> >> questions matter if we're using S2 for CPUidle (where we actually turn
> >> >> off power and hot unplug CPUs) but not when we're using S1 for CPUidle
> >> >> (where we just enter WFI/WFE).
> >> >>
> >>
> >> No Its not plain WFI.
> >>
> >> All cores in Exynos5420 can be powered off independently.
> >> This functionality has been tested.
> >>
> >> Below is the link for the posted patches.
> >>
> >> https://lkml.org/lkml/2014/6/10/194
> >>
> >> And as Nicolas wrote, these patches need MCPM for that.
> >
> > Chander, I cast a look into the code and I have a question
> > (you also told me on CPUidle review that only core off
> > is supported in CPUidle).
> >
> > When you say all cores can be powered off independently, do
> > you also mean that clusters can be powered off (in CPUidle) ?
> >
> > I am pointing this out since in the MCPM backend I noticed:
> >
> > "/* TODO: Turn off the cluster here to save power. */"
> >
> > I do not see any cluster power down request in the down path.
> >
> > If I am wrong, ignore my message, I am just writing to help.
> >
> > If you can only power down cores, but not the clusters on idle,
> > please keep in mind that you are currently cleaning and invalidating
> > the L2 when last man is running and this must not be taken
> > lightly since, if L2 stays on, that's a massive energy waste
> > for nothing.
> >
> > So, if clusters stay up, you _have_ to tweak the MCPM backend slightly
> > to avoid cleaning L2, that's pivotal.
> 
> Hi Lorenzo,
> Cluster shutdown is in progress. Abhilash will add support for that.
> 
> https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg31104.html

Hi Chander,

thanks. So, as a heads-up:

1) if you merge CPUidle support now, as it is it is at least suboptimal, may
   even burn more energy than it saves. Latencies in the bL idle driver
   are likely to be wrong, since they are for cluster shutdown and for
   TC2, not core power gating that should require shorter target_residencies.
   On top of that, L2 is cleaned and invalidated for nothing.
2) when cluster support is merged, you might want to extend the CPUidle
   driver to add an additional state (ie C1 core gating, C2 cluster
   gating) and to do that you should extend the driver and the MCPM
   back-end accordingly, I discussed that with Nico already some time ago
   and actually it should be fairly easy to do but we have got to talk
   about that.

Thank you,
Lorenzo

> 
> 
> >
> > Lorenzo
> >
> >>
> >> >> I believe that in ChromeOS we use S1 CPUidle and that it works fine.
> >> >> We've never implemented S2 that I'm aware of.
> >> >
> >> > You'll have to rely on MCPM for that.  That's probably why it hasn't
> >> > been implemented before.
> >> >
> >> >
> >> > Nicolas
> >> >
> >> > _______________________________________________
> >> > linux-arm-kernel mailing list
> >> > linux-arm-kernel@lists.infradead.org
> >> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> Please read the FAQ at  http://www.tux.org/lkml/
> >>
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Chander Kashyap]

On 11 June 2014 18:45, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Wed, Jun 11, 2014 at 01:14:21PM +0100, Chander Kashyap wrote:
>> On Wed, Jun 11, 2014 at 3:43 PM, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>> > On Wed, Jun 11, 2014 at 05:52:10AM +0100, Chander Kashyap wrote:
>> >> Hi Doug,
>> >>
>> >> On Tue, Jun 10, 2014 at 9:19 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
>> >> > On Tue, 10 Jun 2014, Doug Anderson wrote:
>> >> >
>> >> >> My S-state knowledge is not strong, but I believe that Lorenzo's
>> >> >> questions matter if we're using S2 for CPUidle (where we actually turn
>> >> >> off power and hot unplug CPUs) but not when we're using S1 for CPUidle
>> >> >> (where we just enter WFI/WFE).
>> >> >>
>> >>
>> >> No Its not plain WFI.
>> >>
>> >> All cores in Exynos5420 can be powered off independently.
>> >> This functionality has been tested.
>> >>
>> >> Below is the link for the posted patches.
>> >>
>> >> https://lkml.org/lkml/2014/6/10/194
>> >>
>> >> And as Nicolas wrote, these patches need MCPM for that.
>> >
>> > Chander, I cast a look into the code and I have a question
>> > (you also told me on CPUidle review that only core off
>> > is supported in CPUidle).
>> >
>> > When you say all cores can be powered off independently, do
>> > you also mean that clusters can be powered off (in CPUidle) ?
>> >
>> > I am pointing this out since in the MCPM backend I noticed:
>> >
>> > "/* TODO: Turn off the cluster here to save power. */"
>> >
>> > I do not see any cluster power down request in the down path.
>> >
>> > If I am wrong, ignore my message, I am just writing to help.
>> >
>> > If you can only power down cores, but not the clusters on idle,
>> > please keep in mind that you are currently cleaning and invalidating
>> > the L2 when last man is running and this must not be taken
>> > lightly since, if L2 stays on, that's a massive energy waste
>> > for nothing.
>> >
>> > So, if clusters stay up, you _have_ to tweak the MCPM backend slightly
>> > to avoid cleaning L2, that's pivotal.
>>
>> Hi Lorenzo,
>> Cluster shutdown is in progress. Abhilash will add support for that.
>>
>> https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg31104.html
>

Hi Lorenzo,

> Hi Chander,
>
> thanks. So, as a heads-up:
>
> 1) if you merge CPUidle support now, as it is it is at least suboptimal, may
>    even burn more energy than it saves. Latencies in the bL idle driver
>    are likely to be wrong, since they are for cluster shutdown and for
>    TC2, not core power gating that should require shorter target_residencies.
>    On top of that, L2 is cleaned and invalidated for nothing.

Yes true.

> 2) when cluster support is merged, you might want to extend the CPUidle
>    driver to add an additional state (ie C1 core gating, C2 cluster
>    gating) and to do that you should extend the driver and the MCPM
>    back-end accordingly, I discussed that with Nico already some time ago
>    and actually it should be fairly easy to do but we have got to talk
>    about that.
>

Yes thats the final goal to add two states (C1 core gating, C2 cluster gating).
But that will be done in combination with yours patches(to pass
cpuidle data thru DT).

So finally the mcpm backend will take care for c1 and c2 cpuidle state
implementation.

> Thank you,
> Lorenzo
>
>>
>>
>> >
>> > Lorenzo
>> >
>> >>
>> >> >> I believe that in ChromeOS we use S1 CPUidle and that it works fine.
>> >> >> We've never implemented S2 that I'm aware of.
>> >> >
>> >> > You'll have to rely on MCPM for that.  That's probably why it hasn't
>> >> > been implemented before.
>> >> >
>> >> >
>> >> > Nicolas
>> >> >
>> >> > _______________________________________________
>> >> > linux-arm-kernel mailing list
>> >> > linux-arm-kernel@lists.infradead.org
>> >> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>> >> --
>> >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> >> the body of a message to majordomo@vger.kernel.org
>> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >> Please read the FAQ at  http://www.tux.org/lkml/
>> >>
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>



-- 
with warm regards,
Chander Kashyap

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Abhilash Kesavan]

Hi Lorenzo and Chander,

On Wed, Jun 11, 2014 at 6:45 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Wed, Jun 11, 2014 at 01:14:21PM +0100, Chander Kashyap wrote:
>> On Wed, Jun 11, 2014 at 3:43 PM, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>> > On Wed, Jun 11, 2014 at 05:52:10AM +0100, Chander Kashyap wrote:
>> >> Hi Doug,
>> >>
>> >> On Tue, Jun 10, 2014 at 9:19 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
>> >> > On Tue, 10 Jun 2014, Doug Anderson wrote:
>> >> >
>> >> >> My S-state knowledge is not strong, but I believe that Lorenzo's
>> >> >> questions matter if we're using S2 for CPUidle (where we actually turn
>> >> >> off power and hot unplug CPUs) but not when we're using S1 for CPUidle
>> >> >> (where we just enter WFI/WFE).
>> >> >>
>> >>
>> >> No Its not plain WFI.
>> >>
>> >> All cores in Exynos5420 can be powered off independently.
>> >> This functionality has been tested.
>> >>
>> >> Below is the link for the posted patches.
>> >>
>> >> https://lkml.org/lkml/2014/6/10/194
>> >>
>> >> And as Nicolas wrote, these patches need MCPM for that.
>> >
>> > Chander, I cast a look into the code and I have a question
>> > (you also told me on CPUidle review that only core off
>> > is supported in CPUidle).
>> >
>> > When you say all cores can be powered off independently, do
>> > you also mean that clusters can be powered off (in CPUidle) ?
>> >
>> > I am pointing this out since in the MCPM backend I noticed:
>> >
>> > "/* TODO: Turn off the cluster here to save power. */"
This is something that pends on me.
>> >
>> > I do not see any cluster power down request in the down path.
>> >
>> > If I am wrong, ignore my message, I am just writing to help.
>> >
>> > If you can only power down cores, but not the clusters on idle,
>> > please keep in mind that you are currently cleaning and invalidating
>> > the L2 when last man is running and this must not be taken
>> > lightly since, if L2 stays on, that's a massive energy waste
>> > for nothing.
>> >
>> > So, if clusters stay up, you _have_ to tweak the MCPM backend slightly
>> > to avoid cleaning L2, that's pivotal.
>>
>> Hi Lorenzo,
>> Cluster shutdown is in progress. Abhilash will add support for that.
>>
>> https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg31104.html
>
> Hi Chander,
>
> thanks. So, as a heads-up:
>
> 1) if you merge CPUidle support now, as it is it is at least suboptimal, may
>    even burn more energy than it saves. Latencies in the bL idle driver
>    are likely to be wrong, since they are for cluster shutdown and for
>    TC2, not core power gating that should require shorter target_residencies.
>    On top of that, L2 is cleaned and invalidated for nothing.
> 2) when cluster support is merged, you might want to extend the CPUidle
>    driver to add an additional state (ie C1 core gating, C2 cluster
>    gating) and to do that you should extend the driver and the MCPM
>    back-end accordingly, I discussed that with Nico already some time ago
>    and actually it should be fairly easy to do but we have got to talk
>    about that.

I have already implemented the missing cluster power down code and
tested it on an exynos5420 based chromebook. I plan to post the patch
as soon as I am back in office, which should be early next week.

Regards,
Abhilash
>
> Thank you,
> Lorenzo
>
>>
>>
>> >
>> > Lorenzo
>> >
>> >>
>> >> >> I believe that in ChromeOS we use S1 CPUidle and that it works fine.
>> >> >> We've never implemented S2 that I'm aware of.
>> >> >
>> >> > You'll have to rely on MCPM for that.  That's probably why it hasn't
>> >> > been implemented before.
>> >> >
>> >> >
>> >> > Nicolas
>> >> >
>> >> > _______________________________________________
>> >> > linux-arm-kernel mailing list
>> >> > linux-arm-kernel@lists.infradead.org
>> >> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>> >> --
>> >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> >> the body of a message to majordomo@vger.kernel.org
>> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >> Please read the FAQ at  http://www.tux.org/lkml/
>> >>
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Doug Anderson]

Chander,

On Tue, Jun 10, 2014 at 9:52 PM, Chander Kashyap <k.chander@samsung.com> wrote:
> Hi Doug,
>
> On Tue, Jun 10, 2014 at 9:19 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
>> On Tue, 10 Jun 2014, Doug Anderson wrote:
>>
>>> My S-state knowledge is not strong, but I believe that Lorenzo's
>>> questions matter if we're using S2 for CPUidle (where we actually turn
>>> off power and hot unplug CPUs) but not when we're using S1 for CPUidle
>>> (where we just enter WFI/WFE).
>>>
>
> No Its not plain WFI.
>
> All cores in Exynos5420 can be powered off independently.
> This functionality has been tested.
>
> Below is the link for the posted patches.
>
> https://lkml.org/lkml/2014/6/10/194
>
> And as Nicolas wrote, these patches need MCPM for that.

Most excellent!  I should have been more clear that I only knew about
how CPUidle worked in our local production kernel.  There I'm pretty
sure CPUidle is just WFI/WFE.  If you've got patches to do better then
that's great!

...can you confirm that my patch doesn't interfere with your improved
CPUidle?  It's been Acked by Nicolas (thanks!) so I'd imagine it will
land shortly.  Kukjin: I assume you'll be taking this?

-Doug

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Kukjin Kim]

On 06/12/14 00:19, Doug Anderson wrote:
> Chander,
>
> On Tue, Jun 10, 2014 at 9:52 PM, Chander Kashyap<k.chander@samsung.com>  wrote:
>> Hi Doug,
>>
>> On Tue, Jun 10, 2014 at 9:19 PM, Nicolas Pitre<nicolas.pitre@linaro.org>  wrote:
>>> On Tue, 10 Jun 2014, Doug Anderson wrote:
>>>
>>>> My S-state knowledge is not strong, but I believe that Lorenzo's
>>>> questions matter if we're using S2 for CPUidle (where we actually turn
>>>> off power and hot unplug CPUs) but not when we're using S1 for CPUidle
>>>> (where we just enter WFI/WFE).
>>>>
>>
>> No Its not plain WFI.
>>
>> All cores in Exynos5420 can be powered off independently.
>> This functionality has been tested.
>>
>> Below is the link for the posted patches.
>>
>> https://lkml.org/lkml/2014/6/10/194
>>
>> And as Nicolas wrote, these patches need MCPM for that.
>
> Most excellent!  I should have been more clear that I only knew about
> how CPUidle worked in our local production kernel.  There I'm pretty
> sure CPUidle is just WFI/WFE.  If you've got patches to do better then
> that's great!
>
> ...can you confirm that my patch doesn't interfere with your improved
> CPUidle?  It's been Acked by Nicolas (thanks!) so I'd imagine it will
> land shortly.  Kukjin: I assume you'll be taking this?
>
Sure, I will ;-)

Thanks,
Kukjin

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Chander Kashyap]

On Wed, Jun 11, 2014 at 8:58 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> On 06/12/14 00:19, Doug Anderson wrote:
>>
>> Chander,
>>
>> On Tue, Jun 10, 2014 at 9:52 PM, Chander Kashyap<k.chander@samsung.com>
>> wrote:
>>>
>>> Hi Doug,
>>>
>>> On Tue, Jun 10, 2014 at 9:19 PM, Nicolas Pitre<nicolas.pitre@linaro.org>
>>> wrote:
>>>>
>>>> On Tue, 10 Jun 2014, Doug Anderson wrote:
>>>>
>>>>> My S-state knowledge is not strong, but I believe that Lorenzo's
>>>>> questions matter if we're using S2 for CPUidle (where we actually turn
>>>>> off power and hot unplug CPUs) but not when we're using S1 for CPUidle
>>>>> (where we just enter WFI/WFE).
>>>>>
>>>
>>> No Its not plain WFI.
>>>
>>> All cores in Exynos5420 can be powered off independently.
>>> This functionality has been tested.
>>>
>>> Below is the link for the posted patches.
>>>
>>> https://lkml.org/lkml/2014/6/10/194
>>>
>>> And as Nicolas wrote, these patches need MCPM for that.
>>
>>
>> Most excellent!  I should have been more clear that I only knew about
>> how CPUidle worked in our local production kernel.  There I'm pretty
>> sure CPUidle is just WFI/WFE.  If you've got patches to do better then
>> that's great!
>>
>> ...can you confirm that my patch doesn't interfere with your improved
>> CPUidle?  It's been Acked by Nicolas (thanks!) so I'd imagine it will
>> land shortly.  Kukjin: I assume you'll be taking this?
>>

This patch is effectively changing the mcpm_entry_point address from
nsbase + 0x1c to nsbase + 0x8

Hence while integrating with mainline u-boot we need to take care for
new mcpm_entry_point address.

With Chromebook it works straightforward.


> Sure, I will ;-)
>
> Thanks,
> Kukjin
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Nicolas Pitre]

On Fri, 13 Jun 2014, Chander Kashyap wrote:

> This patch is effectively changing the mcpm_entry_point address from
> nsbase + 0x1c to nsbase + 0x8
> 
> Hence while integrating with mainline u-boot we need to take care for
> new mcpm_entry_point address.

Why not inserting a NOP as the first instruction then? That way the 
offset will remain the same.


Nicolas

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Doug Anderson]

Chander,

On Fri, Jun 13, 2014 at 4:54 AM, Chander Kashyap <k.chander@samsung.com> wrote:
> This patch is effectively changing the mcpm_entry_point address from
> nsbase + 0x1c to nsbase + 0x8
>
> Hence while integrating with mainline u-boot we need to take care for
> new mcpm_entry_point address.
>
> With Chromebook it works straightforward.

Can you explain more and point to the code that is using the nsbase +
0x1c?  Specifically the only code I see that uses the nsbase + 0x1c is
the code that is located at nsbase, which is the code we're
overwriting here.  I'd imagine you're using U-Boot code that looks
something like the bits that start at code_base here:

https://chromium.googlesource.com/chromiumos/third_party/u-boot/+/ce358daf5069f1dc145b0f9d403cfbb028271807/arch/arm/cpu/armv7/exynos/lowlevel.S

With my kernel change you can completely eliminate U-Boot's
installation of this code (or keep it, it makes no difference).

-Doug

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Chander Kashyap]

Hi Doug,

On 13 June 2014 20:40, Doug Anderson <dianders@chromium.org> wrote:
> Chander,
>
> On Fri, Jun 13, 2014 at 4:54 AM, Chander Kashyap <k.chander@samsung.com> wrote:
>> This patch is effectively changing the mcpm_entry_point address from
>> nsbase + 0x1c to nsbase + 0x8
>>
>> Hence while integrating with mainline u-boot we need to take care for
>> new mcpm_entry_point address.
>>
>> With Chromebook it works straightforward.
>
> Can you explain more and point to the code that is using the nsbase +
> 0x1c?  Specifically the only code I see that uses the nsbase + 0x1c is
> the code that is located at nsbase, which is the code we're
> overwriting here.  I'd imagine you're using U-Boot code that looks
> something like the bits that start at code_base here:
>
> https://chromium.googlesource.com/chromiumos/third_party/u-boot/+/ce358daf5069f1dc145b0f9d403cfbb028271807/arch/arm/cpu/armv7/exynos/lowlevel.S
>
> With my kernel change you can completely eliminate U-Boot's
> installation of this code (or keep it, it makes no difference).

Yes i agree with your point.
What i am saying is when there is full support for Exynos5420 in
mainline u-boot we need to take care for the mcpm_entry_point address.

>
> -Doug



-- 
with warm regards,
Chander Kashyap

Re: [PATCH] ARM: EXYNOS: mcpm: Don't rely on firmware's secondary_cpu_start

[Doug Anderson]

Kukjin,

On Wed, Jun 11, 2014 at 8:28 AM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> On 06/12/14 00:19, Doug Anderson wrote:
>>
>> Chander,
>>
>> On Tue, Jun 10, 2014 at 9:52 PM, Chander Kashyap<k.chander@samsung.com>
>> wrote:
>>>
>>> Hi Doug,
>>>
>>> On Tue, Jun 10, 2014 at 9:19 PM, Nicolas Pitre<nicolas.pitre@linaro.org>
>>> wrote:
>>>>
>>>> On Tue, 10 Jun 2014, Doug Anderson wrote:
>>>>
>>>>> My S-state knowledge is not strong, but I believe that Lorenzo's
>>>>> questions matter if we're using S2 for CPUidle (where we actually turn
>>>>> off power and hot unplug CPUs) but not when we're using S1 for CPUidle
>>>>> (where we just enter WFI/WFE).
>>>>>
>>>
>>> No Its not plain WFI.
>>>
>>> All cores in Exynos5420 can be powered off independently.
>>> This functionality has been tested.
>>>
>>> Below is the link for the posted patches.
>>>
>>> https://lkml.org/lkml/2014/6/10/194
>>>
>>> And as Nicolas wrote, these patches need MCPM for that.
>>
>>
>> Most excellent!  I should have been more clear that I only knew about
>> how CPUidle worked in our local production kernel.  There I'm pretty
>> sure CPUidle is just WFI/WFE.  If you've got patches to do better then
>> that's great!
>>
>> ...can you confirm that my patch doesn't interfere with your improved
>> CPUidle?  It's been Acked by Nicolas (thanks!) so I'd imagine it will
>> land shortly.  Kukjin: I assume you'll be taking this?
>>
> Sure, I will ;-)

I see that you put some branches up about 3 hours ago and I don't see
this patch.  Are you planning on applying it?  It would be nice if it
was in 3.16 (since it causes problems booting), but if there's some
reason it needs to be for-next that's OK too.

-Doug