RE: [PATCH] ARM: EXYNOS: cpuidle: Skip C1 cpuidle state for exynos5440

[Kukjin Kim <kgene@kernel.org>]

amit daniel kachhap wrote:
> 
> Hi Daniel/Tomasz,
> 
> From the discussion I can conclude that SOC check is needed in the
> cpuidle driver for deeper C states. Only the question is where to
> insert this.
> Also to perform the SOC there can be 2 ways like
> 1) each SOC check 4120, 4412, 5250 etc (long list)
> 2) negate the nonsupporting SOC's like 5440 (small list like current patch)
> Any opinion?

I’d preferred to use 2nd :)

>  As Bartlomiej suggested that this patch conflicts with Daniel's
> earlier patch http://marc.info/?l=linux-arm-kernel&m=137467935712513&w=2
> So I can re-base my patch on top of this one if needed.
> 
Sounds good to me.

Thanks,
Kukjin

> Thanks,
> Amit Daniel
> 
> On Sun, Jul 28, 2013 at 4:01 PM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> > On 07/28/2013 11:22 AM, Tomasz Figa wrote:
> >> On Sunday 28 of July 2013 09:10:09 Daniel Lezcano wrote:
> >>> On 07/24/2013 01:47 PM, Kukjin Kim wrote:
> >>>> Amit Daniel Kachhap wrote:
> >>>>> This patch skips the deep C1(AFTR -Arm off top running) state for
> >>>>> exynos5440
> >>>>> soc as this soc does not support this state. All the cpu's only
> >>>>> allows the basic
> >>>>> C0 state.
> >>>>>
> >>>>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> >>>>> ---
> >>>>>
> >>>>>  arch/arm/mach-exynos/cpuidle.c |    2 +-
> >>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-
> >>>>> exynos/cpuidle.c
> >>>>> index 17a18ff..9a776a1 100644
> >>>>> --- a/arch/arm/mach-exynos/cpuidle.c
> >>>>> +++ b/arch/arm/mach-exynos/cpuidle.c
> >>>>> @@ -210,7 +210,7 @@ static int __init exynos4_init_cpuidle(void)
> >>>>>
> >>>>>            device->cpu = cpu_id;
> >>>>>
> >>>>>            /* Support IDLE only */
> >>>>>
> >>>>> -          if (cpu_id != 0)
> >>>>> +          if (soc_is_exynos5440() || cpu_id != 0)
> >>>>>
> >>>>>                    device->state_count = 1;
> >>>>>
> >>>>>            ret = cpuidle_register_device(device);
> >>>>>
> >>>>> --
> >>>>> 1.7.1
> >>>>
> >>>> Applied, thanks.
> >>>
> >>> You shouldn't have. This patch means exynos5540 has no cpuidle driver
> at
> >>> all. It should be fixed in the Kconfig to unselect CONFIG_CPU_IDLE for
> >>> an exynos5540.
> >>
> >> To shed more light on this, let me add that you need to register a
> cpuidle
> >> driver only if you have more states than a simple WFI or you need some
> >> crazy steps to enter WFI. Default setup falls back to generic ARM WFI.
> >> (Daniel, do we get the nice idle stats as provided by cpuidle core
> then?)
> >
> > Nope, but with one state, idle vs busy stats do the trick.
> >
> > BTW, I am writing a tool to do some stats based on the idle events [1].
> > It is still at a very early development stage but we can get some
> > interesting informations.
> >
> >
> >> Anyway, Exynos cpuidle is using an initcall to initialize and we
> support
> >> multiple Exynos SoCs in single zImage, so deselecting CONFIG_CPU_IDLE
> is
> >> not an option.
> >
> > Good point.
> >
> >> Considering multiplatform requirements, the driver has to
> >> be modified to initialize only on supported platforms, either by:
> >>
> >>  a) dropping the initcall and calling the init function directly from
> >> arch/arm/mach-exynos
> >>
> >>  or
> >>
> >>  b) checking if machine we are running on is supported, which would
> mean a
> >> long list of all Exynos SoCs that needs to be checked.
> >>
> >> An evolution of option a) is registering a platform device somewhere in
> >> arch/arm/mach-exynos and making exynos-cpuidle a platform driver.
> >
> > Yes, I am favorable to this solution [2].
> >
> >> The
> >> problem is that you must register a static platform device from arch
> code,
> >> because cpuidle is not a real hardware block that can be put into
> device
> >> tree.