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

[Kevin Hilman]

Lorenzo Pieralisi <lorenzo.pieralisi at arm.com> writes:

> On Thu, Jan 08, 2015 at 11:11:40AM +0000, Daniel Lezcano wrote:
>
> [...]
>
>> >> 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.
>
>> 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)

Kevin