diff mbox

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

Message ID 1402090985-8061-1-git-send-email-dianders@chromium.org (mailing list archive)
State New, archived
Headers show

Commit Message

Douglas Anderson June 6, 2014, 9:43 p.m. UTC
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(-)

Comments

Nicolas Pitre June 6, 2014, 10:35 p.m. UTC | #1
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
--
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
Douglas Anderson June 6, 2014, 10:43 p.m. UTC | #2
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.
--
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
Andrew Bresticker June 6, 2014, 10:46 p.m. UTC | #3
>> 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.
--
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
Lorenzo Pieralisi June 7, 2014, 6:12 p.m. UTC | #4
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/
> 

--
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
Douglas Anderson June 9, 2014, 5:03 p.m. UTC | #5
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
--
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
Lorenzo Pieralisi June 9, 2014, 10:38 p.m. UTC | #6
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

--
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
Chander Kashyap June 10, 2014, 8:12 a.m. UTC | #7
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
Douglas Anderson June 10, 2014, 3:21 p.m. UTC | #8
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
--
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
Nicolas Pitre June 10, 2014, 3:49 p.m. UTC | #9
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
--
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
Chander Kashyap June 11, 2014, 4:52 a.m. UTC | #10
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
--
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
Lorenzo Pieralisi June 11, 2014, 10:13 a.m. UTC | #11
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/
> 

--
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
Chander Kashyap June 11, 2014, 12:14 p.m. UTC | #12
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
--
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
Lorenzo Pieralisi June 11, 2014, 1:15 p.m. UTC | #13
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
> 

--
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
Chander Kashyap June 11, 2014, 1:29 p.m. UTC | #14
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
>>
>
Abhilash Kesavan June 11, 2014, 1:34 p.m. UTC | #15
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/
--
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
Douglas Anderson June 11, 2014, 3:19 p.m. UTC | #16
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
--
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
Kim Kukjin June 11, 2014, 3:28 p.m. UTC | #17
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
--
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
Chander Kashyap June 13, 2014, 11:54 a.m. UTC | #18
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
--
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
Nicolas Pitre June 13, 2014, 1:29 p.m. UTC | #19
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
--
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
Douglas Anderson June 13, 2014, 3:10 p.m. UTC | #20
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
--
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
Chander Kashyap June 16, 2014, 7:40 a.m. UTC | #21
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
Douglas Anderson June 16, 2014, 6:35 p.m. UTC | #22
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
--
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
diff mbox

Patch

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);