Re: [PATCH 2/2] ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter

[Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>]

On Monday, September 02, 2013 03:18:51 PM Daniel Lezcano wrote:
> On 09/02/2013 11:41 AM, Bartlomiej Zolnierkiewicz wrote:
> > On Monday, September 02, 2013 10:54:17 AM Daniel Lezcano wrote:
> >> On 08/30/2013 12:21 PM, Bartlomiej Zolnierkiewicz wrote:
> >>> Add "cpuidle-exynos.max_states=" parameter to allow user to specify
> >>> the maximum of allowed CPU idle states for ARM EXYNOS cpuidle driver.
> >>>
> >>> This change is needed because C1 state (AFTR mode) is often not able
> >>> to work properly due to incompatibility with some bootloader versions.
> >>>
> >>> Usage examples:
> >>>
> >>> "cpuidle-exynos.max_states=1" disables C1 state (AFTR mode).
> >>>
> >>> "cpuidle-exynos.max_states=0" disables the driver completely.
> >>>
> >>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> >>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >>> Cc: Tomasz Figa <t.figa@samsung.com>
> >>> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
> >>
> >> There is a max_cstate option for acpi and intel idle. There is also the
> >> cpuidle.off=1 option. As the semantic is the same, I think adding a
> >> common cpuidle option usable for all the drivers is better.
> > 
> > I thought about making the option common for all cpuidle drivers first
> > but due to support for multiple cpuidle drivers on one machine (i.e.
> > big.LITTLE), per-driver option looked like a better approach.
> > 
> > Should I make the option common and not worry about multiple drivers on
> > one machine support?
> 
> Mmh, that's a good point.
> 
> I am not in favor of multiple options spread across the different
> drivers. Furthermore the max_cstate is used in the intel platform to
> 'discover' what states the firmware supports which is not the case of
> the cpuidle ARM drivers (except new PSCI based). This option does not
> really fits well here.
> 
> There is the kernel parameter 'cpuidle.off', so disabling the driver is ok.
> 
> You converted the cpuidle driver to a platform driver. Isn't possible to
> pass information in the platform data field at boot time to tell AFTR is
> not supported and then act on the 'disabled' field of this state ?

It might be possible but I don't know where the source of this data would
be, platform specific kernel parameter? It sounds just like moving the code
around and adding superfluous platform->driver code because the similar
kernel parameter to disable just AFTR can be added in cpuidle-exynos driver
as well.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics