Nokia N900: Proper C-states

Nokia N900: Proper C-states

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 616 bytes --]

Hello,

due to this Daniel Lezcano commit (ARM: OMAP3: cpuidle - remove rx51
cpuidle parameters table)

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=231900afba52d6faddfb480cde4132d4edc089bc

we need patch cpuidle34xx.c code to fix commit for Nokia N900. See:

https://github.com/pali/linux-n900/commit/e147fd4b678f1f3d7a5235287910960bd41e04dc

As Nokia N900 code is converting from legacy board code to DST, I would
like to know how to patch correctly omap3_idle_driver in DTS with
correct values measured for Nokia N900. Thanks!

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Nokia N900: Proper C-states

[Daniel Lezcano]

On 01/02/2016 03:26 PM, Pali Rohár wrote:
> Hello,
>
> due to this Daniel Lezcano commit (ARM: OMAP3: cpuidle - remove rx51
> cpuidle parameters table)
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=231900afba52d6faddfb480cde4132d4edc089bc
>
> we need patch cpuidle34xx.c code to fix commit for Nokia N900. See:
>
> https://github.com/pali/linux-n900/commit/e147fd4b678f1f3d7a5235287910960bd41e04dc
>
> As Nokia N900 code is converting from legacy board code to DST, I would
> like to know how to patch correctly omap3_idle_driver in DTS with
> correct values measured for Nokia N900. Thanks!

Hi Pali,

the conversion to the DT based arm cpuidle driver could be a bit complex 
with one issue (index 0 != cpu_do_idle()) and one performance regression 
(cpu_pm_enter/exit will be called in retention mode, even if this is not 
needed).

It will result in a PM code only on one side and on the other side, the 
generic cpuidle-arm.c driver will be used instead with the DT 
definition. It is worth to the conversion because the result will be 
nice IMO.

Added Lorenzo who is initially author of the arm generic driver. We 
already discussed in the past about those two issues above and I think 
this is something we should improve.



-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: Nokia N900: Proper C-states

[Tony Lindgren]

* Daniel Lezcano <daniel.lezcano@linaro.org> [160102 14:01]:
> On 01/02/2016 03:26 PM, Pali Rohár wrote:
> >Hello,
> >
> >due to this Daniel Lezcano commit (ARM: OMAP3: cpuidle - remove rx51
> >cpuidle parameters table)
> >
> >https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=231900afba52d6faddfb480cde4132d4edc089bc
> >
> >we need patch cpuidle34xx.c code to fix commit for Nokia N900. See:
> >
> >https://github.com/pali/linux-n900/commit/e147fd4b678f1f3d7a5235287910960bd41e04dc
> >
> >As Nokia N900 code is converting from legacy board code to DST, I would
> >like to know how to patch correctly omap3_idle_driver in DTS with
> >correct values measured for Nokia N900. Thanks!

I'm almost certain those values are not n900 specific but should be
used for omap3430 in general.

> the conversion to the DT based arm cpuidle driver could be a bit complex
> with one issue (index 0 != cpu_do_idle()) and one performance regression
> (cpu_pm_enter/exit will be called in retention mode, even if this is not
> needed).
> 
> It will result in a PM code only on one side and on the other side, the
> generic cpuidle-arm.c driver will be used instead with the DT definition. It
> is worth to the conversion because the result will be nice IMO.
> 
> Added Lorenzo who is initially author of the arm generic driver. We already
> discussed in the past about those two issues above and I think this is
> something we should improve.

While this conversion process is going on, we could just add
omap3430_idle_driver and set that based on cpu_is_omap3430()
in omap3_idle_init().

This is assuming it really is 3430 specific. The only thing that
could potentially make it n900 specific is the fact that Nokia had
their own bootrom. But AFAIK these latencies are hardware specific
and should not be related to the bootrom.

Regards,

Tony

Re: Nokia N900: Proper C-states

[Daniel Lezcano]

On 02/09/2016 04:56 PM, Tony Lindgren wrote:
> * Daniel Lezcano <daniel.lezcano@linaro.org> [160102 14:01]:
>> On 01/02/2016 03:26 PM, Pali Rohár wrote:
>>> Hello,
>>>
>>> due to this Daniel Lezcano commit (ARM: OMAP3: cpuidle - remove rx51
>>> cpuidle parameters table)
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=231900afba52d6faddfb480cde4132d4edc089bc
>>>
>>> we need patch cpuidle34xx.c code to fix commit for Nokia N900. See:
>>>
>>> https://github.com/pali/linux-n900/commit/e147fd4b678f1f3d7a5235287910960bd41e04dc
>>>
>>> As Nokia N900 code is converting from legacy board code to DST, I would
>>> like to know how to patch correctly omap3_idle_driver in DTS with
>>> correct values measured for Nokia N900. Thanks!
>
> I'm almost certain those values are not n900 specific but should be
> used for omap3430 in general.
>
>> the conversion to the DT based arm cpuidle driver could be a bit complex
>> with one issue (index 0 != cpu_do_idle()) and one performance regression
>> (cpu_pm_enter/exit will be called in retention mode, even if this is not
>> needed).
>>
>> It will result in a PM code only on one side and on the other side, the
>> generic cpuidle-arm.c driver will be used instead with the DT definition. It
>> is worth to the conversion because the result will be nice IMO.
>>
>> Added Lorenzo who is initially author of the arm generic driver. We already
>> discussed in the past about those two issues above and I think this is
>> something we should improve.
>
> While this conversion process is going on, we could just add
> omap3430_idle_driver and set that based on cpu_is_omap3430()
> in omap3_idle_init().

Yes, that could be a temporary alternative.




-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: Nokia N900: Proper C-states

[Pali Rohár]

On Tuesday 09 February 2016 18:06:26 Daniel Lezcano wrote:
> On 02/09/2016 04:56 PM, Tony Lindgren wrote:
> >* Daniel Lezcano <daniel.lezcano@linaro.org> [160102 14:01]:
> >>On 01/02/2016 03:26 PM, Pali Rohár wrote:
> >>>Hello,
> >>>
> >>>due to this Daniel Lezcano commit (ARM: OMAP3: cpuidle - remove rx51
> >>>cpuidle parameters table)
> >>>
> >>>https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=231900afba52d6faddfb480cde4132d4edc089bc
> >>>
> >>>we need patch cpuidle34xx.c code to fix commit for Nokia N900. See:
> >>>
> >>>https://github.com/pali/linux-n900/commit/e147fd4b678f1f3d7a5235287910960bd41e04dc
> >>>
> >>>As Nokia N900 code is converting from legacy board code to DST, I would
> >>>like to know how to patch correctly omap3_idle_driver in DTS with
> >>>correct values measured for Nokia N900. Thanks!
> >
> >I'm almost certain those values are not n900 specific but should be
> >used for omap3430 in general.
> >

cpuidle parameters for Nokia N900 were added by this commit:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5a1b1d3a9efad6bd53d01ff02e86626d1a51d697

which says:

Numbers based on measurements made in October 2009 for PM optimized
kernel with CPU freq enabled.

Tony, do you have any other omap3430 device?

> >>the conversion to the DT based arm cpuidle driver could be a bit complex
> >>with one issue (index 0 != cpu_do_idle()) and one performance regression
> >>(cpu_pm_enter/exit will be called in retention mode, even if this is not
> >>needed).
> >>
> >>It will result in a PM code only on one side and on the other side, the
> >>generic cpuidle-arm.c driver will be used instead with the DT definition. It
> >>is worth to the conversion because the result will be nice IMO.
> >>
> >>Added Lorenzo who is initially author of the arm generic driver. We already
> >>discussed in the past about those two issues above and I think this is
> >>something we should improve.
> >
> >While this conversion process is going on, we could just add
> >omap3430_idle_driver and set that based on cpu_is_omap3430()
> >in omap3_idle_init().
> 
> Yes, that could be a temporary alternative.

I would be happy with any solution that allows me to drop my hack patch
from linux-n900 tree.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: Nokia N900: Proper C-states

[Tony Lindgren]

* Pali Rohár <pali.rohar@gmail.com> [160211 07:03]:
> > On 02/09/2016 04:56 PM, Tony Lindgren wrote:
> > >I'm almost certain those values are not n900 specific but should be
> > >used for omap3430 in general.
> 
> cpuidle parameters for Nokia N900 were added by this commit:
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5a1b1d3a9efad6bd53d01ff02e86626d1a51d697
> 
> which says:
> 
> Numbers based on measurements made in October 2009 for PM optimized
> kernel with CPU freq enabled.
> 
> Tony, do you have any other omap3430 device?

Yes I can make sure it works for omap3-ldp for a minimal case.
That's a 3430 ES 3.0 that's a GP SoC.

Regards,

Tony

Re: Nokia N900: Proper C-states

[Pali Rohár]

On Thursday 11 February 2016 09:47:50 Tony Lindgren wrote:
> * Pali Rohár <pali.rohar@gmail.com> [160211 07:03]:
> > > On 02/09/2016 04:56 PM, Tony Lindgren wrote:
> > > >I'm almost certain those values are not n900 specific but should be
> > > >used for omap3430 in general.
> > 
> > cpuidle parameters for Nokia N900 were added by this commit:
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5a1b1d3a9efad6bd53d01ff02e86626d1a51d697
> > 
> > which says:
> > 
> > Numbers based on measurements made in October 2009 for PM optimized
> > kernel with CPU freq enabled.
> > 
> > Tony, do you have any other omap3430 device?
> 
> Yes I can make sure it works for omap3-ldp for a minimal case.
> That's a 3430 ES 3.0 that's a GP SoC.
> 
> Regards,
> 
> Tony

Ok. If it works and you think those values are not Nokia N900 specific,
but omap3430, we can add them into kernel.

Do you want me to prepare patch? Or will you do it?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: Nokia N900: Proper C-states

[Pali Rohár]

On Thursday 11 February 2016 16:02:21 Pali Rohár wrote:
> On Tuesday 09 February 2016 18:06:26 Daniel Lezcano wrote:
> > On 02/09/2016 04:56 PM, Tony Lindgren wrote:
> > >* Daniel Lezcano <daniel.lezcano@linaro.org> [160102 14:01]:
> > >>On 01/02/2016 03:26 PM, Pali Rohár wrote:
> > >>>Hello,
> > >>>
> > >>>due to this Daniel Lezcano commit (ARM: OMAP3: cpuidle - remove rx51
> > >>>cpuidle parameters table)
> > >>>
> > >>>https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=231900afba52d6faddfb480cde4132d4edc089bc
> > >>>
> > >>>we need patch cpuidle34xx.c code to fix commit for Nokia N900. See:
> > >>>
> > >>>https://github.com/pali/linux-n900/commit/e147fd4b678f1f3d7a5235287910960bd41e04dc
> > >>>
> > >>>As Nokia N900 code is converting from legacy board code to DST, I would
> > >>>like to know how to patch correctly omap3_idle_driver in DTS with
> > >>>correct values measured for Nokia N900. Thanks!
> > >
> > >I'm almost certain those values are not n900 specific but should be
> > >used for omap3430 in general.
> > >
> 
> cpuidle parameters for Nokia N900 were added by this commit:
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5a1b1d3a9efad6bd53d01ff02e86626d1a51d697
> 
> which says:
> 
> Numbers based on measurements made in October 2009 for PM optimized
> kernel with CPU freq enabled.

Adding Kalle Jokiniemi, author of that commit which added cpuidle
parameters for Nokia N900.

Kalle, do you know if cpuidle values measured are Nokia N900 specific or
general omap3430?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: Nokia N900: Proper C-states

[Tony Lindgren]

* Pali Rohár <pali.rohar@gmail.com> [160215 02:12]:
> On Thursday 11 February 2016 09:47:50 Tony Lindgren wrote:
> > * Pali Rohár <pali.rohar@gmail.com> [160211 07:03]:
> > > > On 02/09/2016 04:56 PM, Tony Lindgren wrote:
> > > > >I'm almost certain those values are not n900 specific but should be
> > > > >used for omap3430 in general.
> > > 
> > > cpuidle parameters for Nokia N900 were added by this commit:
> > > 
> > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5a1b1d3a9efad6bd53d01ff02e86626d1a51d697
> > > 
> > > which says:
> > > 
> > > Numbers based on measurements made in October 2009 for PM optimized
> > > kernel with CPU freq enabled.
> > > 
> > > Tony, do you have any other omap3430 device?
> > 
> > Yes I can make sure it works for omap3-ldp for a minimal case.
> > That's a 3430 ES 3.0 that's a GP SoC.
> > 
> > Regards,
> > 
> > Tony
> 
> Ok. If it works and you think those values are not Nokia N900 specific,
> but omap3430, we can add them into kernel.
> 
> Do you want me to prepare patch? Or will you do it?

Yes please do if you can, I have my hands full with other
regressions in Linux next.

Regards,

Tony

[PATCH] omap3: Add cpuidle parameters table for omap3430

[Pali Rohár]

Based on CPU type choose generic omap3 or omap3430 specific cpuidle
parameters. Parameters for omap3430 were measured on Nokia N900 device and
added by commit 5a1b1d3a9efa ("OMAP3: RX-51: Pass cpu idle parameters")
which were later removed by commit 231900afba52 ("ARM: OMAP3: cpuidle -
remove rx51 cpuidle parameters table") due to huge code complexity.

This patch brings cpuidle parameters for omap3430 devices again, but uses
simple condition based on CPU type.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Fixes: 231900afba52d6faddfb480cde4132d4edc089bc
---
 arch/arm/mach-omap2/cpuidle34xx.c |   69 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index aa7b379..2a3db0b 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -34,6 +34,7 @@
 #include "pm.h"
 #include "control.h"
 #include "common.h"
+#include "soc.h"
 
 /* Mach specific information to be recorded in the C-state driver_data */
 struct omap3_idle_statedata {
@@ -315,6 +316,69 @@ static struct cpuidle_driver omap3_idle_driver = {
 	.safe_state_index = 0,
 };
 
+/*
+ * Numbers based on measurements made in October 2009 for PM optimized kernel
+ * with CPU freq enabled on device Nokia N900. Assumes OPP2 (main idle OPP,
+ * and worst case latencies).
+ */
+static struct cpuidle_driver omap3430_idle_driver = {
+	.name             = "omap3430_idle",
+	.owner            = THIS_MODULE,
+	.states = {
+		{
+			.enter		  = omap3_enter_idle_bm,
+			.exit_latency	  = 110 + 162,
+			.target_residency = 5,
+			.name		  = "C1",
+			.desc		  = "MPU ON + CORE ON",
+		},
+		{
+			.enter		  = omap3_enter_idle_bm,
+			.exit_latency	  = 106 + 180,
+			.target_residency = 309,
+			.name		  = "C2",
+			.desc		  = "MPU ON + CORE ON",
+		},
+		{
+			.enter		  = omap3_enter_idle_bm,
+			.exit_latency	  = 107 + 410,
+			.target_residency = 46057,
+			.name		  = "C3",
+			.desc		  = "MPU RET + CORE ON",
+		},
+		{
+			.enter		  = omap3_enter_idle_bm,
+			.exit_latency	  = 121 + 3374,
+			.target_residency = 46057,
+			.name		  = "C4",
+			.desc		  = "MPU OFF + CORE ON",
+		},
+		{
+			.enter		  = omap3_enter_idle_bm,
+			.exit_latency	  = 855 + 1146,
+			.target_residency = 46057,
+			.name		  = "C5",
+			.desc		  = "MPU RET + CORE RET",
+		},
+		{
+			.enter		  = omap3_enter_idle_bm,
+			.exit_latency	  = 7580 + 4134,
+			.target_residency = 484329,
+			.name		  = "C6",
+			.desc		  = "MPU OFF + CORE RET",
+		},
+		{
+			.enter		  = omap3_enter_idle_bm,
+			.exit_latency	  = 7505 + 15274,
+			.target_residency = 484329,
+			.name		  = "C7",
+			.desc		  = "MPU OFF + CORE OFF",
+		},
+	},
+	.state_count = ARRAY_SIZE(omap3_idle_data),
+	.safe_state_index = 0,
+};
+
 /* Public functions */
 
 /**
@@ -333,5 +397,8 @@ int __init omap3_idle_init(void)
 	if (!mpu_pd || !core_pd || !per_pd || !cam_pd)
 		return -ENODEV;
 
-	return cpuidle_register(&omap3_idle_driver, NULL);
+	if (cpu_is_omap3430())
+		return cpuidle_register(&omap3430_idle_driver, NULL);
+	else
+		return cpuidle_register(&omap3_idle_driver, NULL);
 }
-- 
1.7.9.5

Re: [PATCH] omap3: Add cpuidle parameters table for omap3430

[Daniel Lezcano]

On 02/15/2016 07:02 PM, Pali Rohár wrote:
> Based on CPU type choose generic omap3 or omap3430 specific cpuidle
> parameters. Parameters for omap3430 were measured on Nokia N900 device and
> added by commit 5a1b1d3a9efa ("OMAP3: RX-51: Pass cpu idle parameters")
> which were later removed by commit 231900afba52 ("ARM: OMAP3: cpuidle -
> remove rx51 cpuidle parameters table") due to huge code complexity.
>
> This patch brings cpuidle parameters for omap3430 devices again, but uses
> simple condition based on CPU type.
>
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> Fixes: 231900afba52d6faddfb480cde4132d4edc089bc
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] omap3: Add cpuidle parameters table for omap3430

[Pali Rohár]

On Tuesday 16 February 2016 11:01:43 Daniel Lezcano wrote:
> On 02/15/2016 07:02 PM, Pali Rohár wrote:
> >Based on CPU type choose generic omap3 or omap3430 specific cpuidle
> >parameters. Parameters for omap3430 were measured on Nokia N900 device and
> >added by commit 5a1b1d3a9efa ("OMAP3: RX-51: Pass cpu idle parameters")
> >which were later removed by commit 231900afba52 ("ARM: OMAP3: cpuidle -
> >remove rx51 cpuidle parameters table") due to huge code complexity.
> >
> >This patch brings cpuidle parameters for omap3430 devices again, but uses
> >simple condition based on CPU type.
> >
> >Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> >Fixes: 231900afba52d6faddfb480cde4132d4edc089bc
> >---
> 
> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Tony, Nishanth (or somebody else...) can you test this patch on more
omap3 devices to check if it does not break existing cpuidle code?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH] omap3: Add cpuidle parameters table for omap3430

[Tony Lindgren]

* Pali Rohár <pali.rohar@gmail.com> [160216 02:09]:
> On Tuesday 16 February 2016 11:01:43 Daniel Lezcano wrote:
> > On 02/15/2016 07:02 PM, Pali Rohár wrote:
> > >Based on CPU type choose generic omap3 or omap3430 specific cpuidle
> > >parameters. Parameters for omap3430 were measured on Nokia N900 device and
> > >added by commit 5a1b1d3a9efa ("OMAP3: RX-51: Pass cpu idle parameters")
> > >which were later removed by commit 231900afba52 ("ARM: OMAP3: cpuidle -
> > >remove rx51 cpuidle parameters table") due to huge code complexity.
> > >
> > >This patch brings cpuidle parameters for omap3430 devices again, but uses
> > >simple condition based on CPU type.
> > >
> > >Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > >Fixes: 231900afba52d6faddfb480cde4132d4edc089bc
> > >---
> > 
> > Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> Tony, Nishanth (or somebody else...) can you test this patch on more
> omap3 devices to check if it does not break existing cpuidle code?

Seems to work for me on a ldp, which is GP 3430 ES3.0.

Regards,

Tony

Re: [PATCH] omap3: Add cpuidle parameters table for omap3430

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [160216 08:11]:
> * Pali Rohár <pali.rohar@gmail.com> [160216 02:09]:
> > On Tuesday 16 February 2016 11:01:43 Daniel Lezcano wrote:
> > > On 02/15/2016 07:02 PM, Pali Rohár wrote:
> > > >Based on CPU type choose generic omap3 or omap3430 specific cpuidle
> > > >parameters. Parameters for omap3430 were measured on Nokia N900 device and
> > > >added by commit 5a1b1d3a9efa ("OMAP3: RX-51: Pass cpu idle parameters")
> > > >which were later removed by commit 231900afba52 ("ARM: OMAP3: cpuidle -
> > > >remove rx51 cpuidle parameters table") due to huge code complexity.
> > > >
> > > >This patch brings cpuidle parameters for omap3430 devices again, but uses
> > > >simple condition based on CPU type.
> > > >
> > > >Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > >Fixes: 231900afba52d6faddfb480cde4132d4edc089bc
> > > >---
> > > 
> > > Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > 
> > Tony, Nishanth (or somebody else...) can you test this patch on more
> > omap3 devices to check if it does not break existing cpuidle code?
> 
> Seems to work for me on a ldp, which is GP 3430 ES3.0.

OK applying this into omap-for-v4.6/fixes-not-urgent considering
we've already lived with it for years.

Regards,

Tony