[PATCH] drivers: cpuidle: don't initialize big.LITTLE driver if MCPM is unavailable

Daniel Lezcano [off-list ref] writes:

On 01/08/2015 09:27 PM, Kevin Hilman wrote:

quoted

Lorenzo Pieralisi [off-list ref] writes:

quoted

On Thu, Jan 08, 2015 at 11:11:40AM +0000, Daniel Lezcano wrote:

[...]

quoted

quoted

quoted

IMO, it would be better to be more strict with the mcpm
initialization and not let the system boot if something is wrong with
it which I believe is coming from the firmware and let the user to
figure out what is really happening by letting him to disable mcpm in
the kernel configuration (which in turn will disable cpuidle).

Again I fully agree, but in this case I manually switched to legacy boot
mode on TC2 and used same kernel with MCPM config enabled. Do you mean
to say we should not support that even when developer understand the
consequence of that ?

Well, I see there are the exynos5410/5420/5422. For the 5422 on
chromebook2 MCPM works well, IIUC. But for the 5422 on odroid-xu3, MCPM
does not work, hence cpuidle neither because of the firmware.

Silently disabling cpuidle because mcpm did not initialize will hide the
issue.

No. MCPM *will* initialize, Sudeep's patch does not silently disable
CPUidle.
To put it differently MCPM will initialize if CCI is in the DT and it
is "available", so unless defined differently in the dts mcpm will be
available and CPUidle will be initialized (and break if there is an issue
with the platform FW/HW).

I agree the mechanism to define if MCPM is available can be improved
but that's what it is at the moment.

The problem here is to boot a platform with different boot methods
and still have a single kernel image.

quoted

I understand your point about switching to legacy without recompiling
the kernel.

I suggest we add a big fat WARN_ON when the mcpm initialization fails
with your patch.

I think there are multiple facets we are tackling at once here and they
should not be mixed.

1) We left static idle states there to cope with legacy DTBs that were
    published before we introduced idle states bindings. If we want to
    boot eg vexpress in legacy mode but single kernel image with MCPM on,
    we could remove the idle states in DT and the problem would be
    solved; we can't do that since we were forced to leave the static
    idle tables. Overall I think this is not the way to fix the issue.
2) The idle driver should be initialized if there is an idle state entry
    method, which in this case is MCPM. If I boot vexpress with MCPM
    enabled but legacy boot method (ie spin table) with a single kernel image
    I do not want to warn if the idle states entry method (MCPM) can't be
    initialized (and I do not want to get a warning if the idle driver is
    triggering a mcpm_cpu_suspend), so Sudeep's patch is valid and I am
    against adding a:

    if (WARN_ON(!mcpm_is_available())

3) Sudeep's patch is not hiding anything. If CCI is in DT, CCI is
    probed so mcpm_is_available() == true. If the firmware is borked
    the idle states will be entered and we will notice there is something
    wrong

So overall I think Sudeep's patch is sound. I also think we should
improve the way we detect if MCPM is available, and again, I think the
CPU operations on arm64 are a good example that we can and we should
replicate.

This patch disables CPUidle all together, but shouldn't it just disable
the states that rely on MCPM?  IOW, C1 should still work just fine since
it doesn't use MCPM, right?  So, rather than fail the init, it should
just drop any MCPM states (e.g. set ->state_count = 1)

Well, that means we will have a cpuidle driver with the WFI state only
which is the default idle function when there is no cpuidle driver (+
without the governor math).

Ah, OK.  Then it makes more sense to disable the driver all together.

Feel free to add

Acked-by: Kevin Hilman <redacted>

Thanks,

Kevin