* [PATCH] drivers: cpuidle: don't initialize big.LITTLE driver if MCPM is unavailable
2015-01-08 12:29 ` Lorenzo Pieralisi
@ 2015-01-08 14:01 ` Daniel Lezcano
2015-01-08 14:46 ` Lorenzo Pieralisi
2015-01-08 20:27 ` Kevin Hilman
2015-01-09 5:01 ` Sudeep Holla
2 siblings, 1 reply; 14+ messages in thread
From: Daniel Lezcano @ 2015-01-08 14:01 UTC (permalink / raw)
To: linux-arm-kernel
On 01/08/2015 01:29 PM, Lorenzo Pieralisi wrote:
> 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.
Yes, I agree.
> 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.
Ok, I will pick Sudeep's patch. Shall I add your acked-by ?
> I also think we should
> improve the way we detect if MCPM is available,
Yes, I agree.
> and again, I think the
> CPU operations on arm64 are a good example that we can and we should
> replicate.
Ok, I think that will raise another thread soon :)
Thanks for your feedbacks.
-- Daniel
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
<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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] drivers: cpuidle: don't initialize big.LITTLE driver if MCPM is unavailable
2015-01-08 12:29 ` Lorenzo Pieralisi
2015-01-08 14:01 ` Daniel Lezcano
@ 2015-01-08 20:27 ` Kevin Hilman
2015-01-08 20:51 ` Daniel Lezcano
2015-01-09 4:58 ` Sudeep Holla
2015-01-09 5:01 ` Sudeep Holla
2 siblings, 2 replies; 14+ messages in thread
From: Kevin Hilman @ 2015-01-08 20:27 UTC (permalink / raw)
To: linux-arm-kernel
Lorenzo Pieralisi <lorenzo.pieralisi@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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] drivers: cpuidle: don't initialize big.LITTLE driver if MCPM is unavailable
2015-01-08 20:27 ` Kevin Hilman
@ 2015-01-08 20:51 ` Daniel Lezcano
2015-01-09 17:34 ` Kevin Hilman
2015-01-09 4:58 ` Sudeep Holla
1 sibling, 1 reply; 14+ messages in thread
From: Daniel Lezcano @ 2015-01-08 20:51 UTC (permalink / raw)
To: linux-arm-kernel
On 01/08/2015 09:27 PM, Kevin Hilman wrote:
> Lorenzo Pieralisi <lorenzo.pieralisi@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)
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).
--
<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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] drivers: cpuidle: don't initialize big.LITTLE driver if MCPM is unavailable
2015-01-08 20:51 ` Daniel Lezcano
@ 2015-01-09 17:34 ` Kevin Hilman
0 siblings, 0 replies; 14+ messages in thread
From: Kevin Hilman @ 2015-01-09 17:34 UTC (permalink / raw)
To: linux-arm-kernel
Daniel Lezcano <daniel.lezcano@linaro.org> writes:
> On 01/08/2015 09:27 PM, Kevin Hilman wrote:
>> Lorenzo Pieralisi <lorenzo.pieralisi@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)
>
> 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 <khilman@linaro.org>
Thanks,
Kevin
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] drivers: cpuidle: don't initialize big.LITTLE driver if MCPM is unavailable
2015-01-08 20:27 ` Kevin Hilman
2015-01-08 20:51 ` Daniel Lezcano
@ 2015-01-09 4:58 ` Sudeep Holla
1 sibling, 0 replies; 14+ messages in thread
From: Sudeep Holla @ 2015-01-09 4:58 UTC (permalink / raw)
To: linux-arm-kernel
Hi Kevin,
On Friday 09 January 2015 01:57 AM, Kevin Hilman wrote:
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:
>
>> On Thu, Jan 08, 2015 at 11:11:40AM +0000, Daniel Lezcano wrote:
>>
[...]
>> 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)
>
As Daniel pointed out, if there's no cpuidle driver or if cpuidle fails
to choose a convenient idle state, we fall back to the default arch idle
method(arch_cpu_idle)
Regards,
Sudeep
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] drivers: cpuidle: don't initialize big.LITTLE driver if MCPM is unavailable
2015-01-08 12:29 ` Lorenzo Pieralisi
2015-01-08 14:01 ` Daniel Lezcano
2015-01-08 20:27 ` Kevin Hilman
@ 2015-01-09 5:01 ` Sudeep Holla
2 siblings, 0 replies; 14+ messages in thread
From: Sudeep Holla @ 2015-01-09 5:01 UTC (permalink / raw)
To: linux-arm-kernel
Hi Lorenzo,
On Thursday 08 January 2015 05:59 PM, Lorenzo Pieralisi wrote:
> 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.
>
I agree, I could have been more clear on which problem I was fixing.
> 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.
Thanks for the providing clarification in details.
Regards,
Sudeep
^ permalink raw reply [flat|nested] 14+ messages in thread