Hi Magnus, Mark, Lorenzo
Thank you for your comment.
(2015/04/17 23:37), Lorenzo Pieralisi wrote:
> On Fri, Apr 17, 2015 at 02:11:24PM +0100, Magnus Damm wrote:
>> Hi Mark,
>>
>> On Fri, Apr 17, 2015 at 9:46 PM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
>>> On Thu, Apr 16, 2015 at 11:35:38AM +0100, Keita Kobayashi wrote:
>>>> This patch add the ARM CPUs Device Tree binding to document a new
>>>> enable method of Renesas cpuidle.
>>>>
>>>> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@xxxxxxxxxxx>
>>>> ---
>>>> Documentation/devicetree/bindings/arm/cpus.txt | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
>>>> index 8b9e0a9..663ee11 100644
>>>> --- a/Documentation/devicetree/bindings/arm/cpus.txt
>>>> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
>>>> @@ -196,6 +196,7 @@ nodes to be present and contain the properties described below.
>>>> "qcom,gcc-msm8660"
>>>> "qcom,kpss-acc-v1"
>>>> "qcom,kpss-acc-v2"
>>>> + "renesas,rcar-idle"
>>>
>>> The enable-method is about how to bring the CPU up (and potentially
>>> implies other things, like how to take it down again).
>>
>> Thanks for your clarification. I now understand that this is related
>> to SMP boot and CPU Hotplug.
>>
>>> These is no code in this series to that effect, no semantics are
>>> provided, and the name implies this is idle-specific.
>>>
>>> So this doesn't look right, and makes no sense to me.
>>
>> Ok, I somehow (incorrectly?) assumed that the following line in patch
>> 2/5* tied into this:
>> +CPUIDLE_METHOD_OF_DECLARE(rcar, "renesas,rcar-idle", &rcar_cpuidle_ops);
>>
>> [*] [RFC/PATCH v2 2/5] ARM: shmobile: Add cpuidle_ops for R-Car cpuidle
>>
>> But if it is unrelated it should of course be dropped or reworked.
>
> It is related, but Mark's remark is correct. We have to keep in mind
> that an enable-method defines how a CPU is powered {up/down} and
> also suspended (quiesced through idle).
>
> It has to be defined through proper bindings and related code, adding a
> random compatible string for the sake of matching does not cut it,
> that is not acceptable and I stated it from the beginning.
>
> So, to make it clear, an enable-method defines CPU operations as a
> whole, cpu init, power{up/down} and suspend.
>
> It must be documented with proper bindings that defines compatible string and
> related properties.
>
> The sooner we incorporate the CPUidle ops into SMP ops the better to
> prevent this abuse from happening, an enable-method encompasses SMP
> ops and CPUidle ops, actually they must be merged because they represent
> the enable-method implementation as a whole.
>
> I hope this helps.
>
> Lorenzo
I will rename "renesas,rcar-idle" in the next patch.
Regards.
Keita Kobayashi
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
