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

[Bartlomiej Zolnierkiewicz]

On Monday, September 02, 2013 05:52:15 PM Daniel Lezcano wrote:
> On 09/02/2013 04:43 PM, Bartlomiej Zolnierkiewicz wrote:
> > On Monday, September 02, 2013 04:24:23 PM Daniel Lezcano wrote:
> >> On 09/02/2013 03:48 PM, Bartlomiej Zolnierkiewicz wrote:
> >>> 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 at samsung.com>
> >>>>>>> Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
> >>>>>>> Cc: Tomasz Figa <t.figa at samsung.com>
> >>>>>>> Cc: Amit Daniel Kachhap <amit.daniel at 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.
> >>
> >> It is to prevent to add a new kernel parameter (with the documentation)
> >> for a single driver which has a bogus idle state. If that could be
> >> handled internally that would be cleaner.
> > 
> > If I believed that it could be handled internally I wouldn't be trying to
> > add a kernel parameter to handle it.. Would I? ;)
> > 
> >> Can you shortly describe what happens with the bootloader and AFTR ?
> > 
> > AFTR just doesn't work with the custom U-Boot version that we are using
> > (attempts to go into AFTR mode result in lockup) and using the upstream
> > version of U-Boot is not an option since it doesn't support the hardware
> > that we are using AFAIK. I also don't know exactly why it doesn't work
> > (I just suspect that it reuses INFORM registers for some other purposes).
> 
> You want to add a kernel option as a work around for a bug in U-Boot ?
> 
> IMO, you should drop the hot potato to the u-boot guys :)

Well, if it only was so simple. :)

[ Unfortunately, they have higher priority tasks so this is unlikely to get
  fixed in the foreseeable future. ]

Problems with AFTR are also not limited to our U-Boot problem (i.e. issue
with Hypervisor enabled on EXYNOS5 or problem with CONFIG_THUMB2_KERNEL=y
compiled kernels) so it would be nice to have a way to disable it without
the need for recompiling the kernel.

Limiting maximum allowed cpuidle state is also useful for testing purposes
(this is not limited to EXYNOS driver but a generally useful thing).

> >> I guess you are not interested in cpuidle.off=1 because you want cpuidle
> >> statistics for WFI, right ?
> > 
> > Right. :)

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