Re: [RFC/PATCH v2 3/5] devicetree: bindings: Add new cpuidle enable method for Renesas R-car SoCs

[keita kobayashi <keita.kobayashi.ym@renesas.com>]

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@arm.com> 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@renesas.com>
>>>> ---
>>>>  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