On Monday 29 of July 2013 10:16:14 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. A check is needed for the whole cpuidle driver, so it registers only on Exynos SoCs which support deeper C states. > Only the question is where to > insert this. Exynos doesn't support multiplatform yet, but we must make sure that any code being added is multiplatform-aware. So initcall is not a good idea. I would put this somewhere on Exynos-specific initialization path, i.e. something that would not called for all platforms compiled in (in case of multiplatform). As I discussed with Daniel, this should be using a traditional platform_driver model, with the difference that it can't be registered from device tree, but rather statically in mach code. For example, you can add an exynos_register_cpuidle() function in arch/arm/mach-exynos/common.c, which registers such platform device and always call it from exynos4_dt_machine_init() in mach-exynos4-dt.c. In mach-exynos5-dt.c you could make this conditional and check if !soc_is_exynos5440(). Best regards, Tomasz > 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? > 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. > > Thanks, > Amit Daniel > > On Sun, Jul 28, 2013 at 4:01 PM, Daniel Lezcano > > <daniel.lezcano@xxxxxxxxxx> 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@xxxxxxxxxxx> > >>>>> --- > >>>>> > >>>>> 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. > > > > Thanks > > > > -- Daniel > > > > [1] > > https://git.linaro.org/gitweb?p=people/dlezcano/idlestat.git;a=summar > > y [2] http://patches.linaro.org/18368/ > > > > -- > > > > <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 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe > > linux-samsung-soc" in the body of a message to > > majordomo@xxxxxxxxxxxxxxx > > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |