Re: [PATCH RESEND] devicetree: bindings: separate CPU enable method descriptions

From: Alex Elder
Date: Tue Jun 03 2014 - 12:54:05 EST

On 06/03/2014 09:42 AM, Mark Rutland wrote:
> On Tue, Jun 03, 2014 at 02:48:18PM +0100, Alex Elder wrote:
>> On 06/03/2014 05:08 AM, Mark Rutland wrote:
>>> Hi Alex,
>>>
>>> On Fri, May 30, 2014 at 11:11:54PM +0100, Alex Elder wrote:
>>>> The bindings for CPU enable methods are defined in ".../arm/cpus.txt". As
>>>> additional 32-bit ARM CPUS are converted to use the "enable-method" CPU
>>>> property to imply a particular set of SMP operations to use, the list of these
>>>> methods is likely to become unwieldy. The current documentation already
>>>> contains several property descriptions that are meaningful only for certain
>>>> enable methods.
>>>>
>>>> This patch defines a new Documentation subdirectory whose purpose is to give
>>>> each CPU enable method its own place to define how and when it's used, as
>>>> well as what other properties (optional or required) are associated with
>>>> the method. The existing enable method documentation is expanded and moved
>>>> from ".../arm/cpus.txt" into new files accordingly.
>>>>
>>>> Signed-off-by: Alex Elder <elder@xxxxxxxxxx>
>>>> ---
>>>> v2: Rename "arm,psci.txt" to be "psci.txt" and fix its content
>>>>

. . .

>>>> diff --git a/Documentation/devicetree/bindings/arm/cpu-enable-method/psci.txt b/Documentation/devicetree/bindings/arm/cpu-enable-method/psci.txt
>>>> new file mode 100644
>>>> index 0000000..68b26c2
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/arm/cpu-enable-method/psci.txt
>>>> @@ -0,0 +1,45 @@
>>>> +================================
>>>> +CPU enable-method "psci" binding
>>>> +================================
>>>> +
>>>> +This document describes the "psci" method for enabling secondary CPUs. A
>>>> +"psci" enable method is supported only in individual "cpu" nodes (even if
>>>> + all CPU cores use the "psci" enable method).
>>>> +
>>>> +Enable method name: "psci"
>>>> +Compatible cpus: "arm,cortex-a57" (?)
>>>
>>> Any CPU with the security extensions or hypervisor extensions may
>>> support PSCI. So far, implementations exist for at Cortex-A{7,15,53,57},
>>> in addition to AEMs.
>>
>> I'd like to capture this, but I don't have the information
>> I need. What I'm trying to do is use the actual matching
>> "compatible" string here, and unfortunately there aren't many
>> of them in the upstream tree (xenvm-4.2 and ex-common, and the
>> latter doesn't even have a "cpus" node). I can, from that,
>> include "arm,cortex-a15" however.
>
> I'm not sure I follow why that would be useful.
>
> PSCI can be implemented on any CPU with the security extensions or the
> virtualization extensions. The exact CPU doesn't matter. The code that
> uses them is generic, and is already in the tree. Neither the standard
> nor the kernel code care about the particular CPU.

This is why I'm not really the person who should be
populating this information... I don't know this
stuff. I just split the documentation that now lies
in the "cpus.txt" file into separate files, and tried
my best to flesh out that documentation a bit. (Maybe
that was a mistake.)

I am going to send an update today, like I said I would,
and hopefully move this closer to being acceptable. But
I really need some detailed help from people making sure
the content is right. That probably means either supplying
some suggested wording, or teaching me about it so I can
try to word it well.

My main objective was to separate this documentation
from "cpus.txt." Improving on what's there is beyond
that scope, but I'll do my best.

>> If you have specific values I can add please provide them.
>> On the other hand, perhaps they should be added to the
>> document when the code that uses them lands in the tree.
>
> No PSCI code is going to rely on the CPU compatible string. In the case
> of firmware bugs, we can add properties to the psci node or some PSCI
> implementation detection. The CPU doesn't matter.
>
> The DTBs or code which generates the DTBs which contain PSCI nodes might
> not even be in the kernel tree (see Xen), and it I don't see why we
> should add documentation to Linux when an external project implements
> something that we already support.
>
>>
>>>> +Related properties: (none)
>>>> +
>>>> +Note:
>>>> +This enable method is only available if a valid PSCI node[1] (compatible
>>>> +with "arm,psci") is present in the device tree, and it defines a "cpu_on"
>>>> +property.
>>>> +
>>>> +Example (contrived 2-core ARM Cortex-A57 64-bit system):
>>>> +
>>>> + psci {
>>>> + compatible = "arm,psci";
>>>> + method = "smc";
>>>> + cpu_on = 0x1;
>>>
>>> Missing '<' and '>'.
>>
>> Will fix.
>>
>>>
>>>> + };
>>>> + cpus {
>>>> + #size-cells = <0>;
>>>> + #address-cells = <2>;
>>>> +
>>>> + cpu@0 {
>>>> + device_type = "cpu";
>>>> + compatible = "arm,cortex-a57";
>>>> + reg = <0x0 0x0>;
>>>> + enable-method = "psci";
>>>> + };
>>>> +
>>>> + cpu@1 {
>>>> + device_type = "cpu";
>>>> + compatible = "arm,cortex-a57";
>>>> + reg = <0x0 0x1>;
>>>> + enable-method = "psci";
>>>> + };
>>>> + };
>>>> +
>>>> +--
>>>> +[1] arm/psci.txt
>>>
>>> It may be worth folding the existing PSCI binding document in with the
>>> enable-method portion.
>>
>> I'll see what I can do.
>
> Cheers!
>
>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/cpu-enable-method/qcom,gcc-msm8660 b/Documentation/devicetree/bindings/arm/cpu-enable-method/qcom,gcc-msm8660
>>>> new file mode 100644
>>>> index 0000000..b19f51c
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/arm/cpu-enable-method/qcom,gcc-msm8660
>>>> @@ -0,0 +1,30 @@
>>>> +======================================================
>>>> +Secondary CPU enable-method "qcom,gcc-msm8660" binding
>>>> +======================================================
>>>> +
>>>> +This document describes the "qcom,gcc-msm8660" method for enabling secondary
>>>> +CPUs. A "qcom,gcc-msm8660" enable method should only be used in the "cpus"
>>>> +node, to apply to all CPUs.
>>>> +
>>>> +Enable method name: "qcom,gcc-msm8660"
>>>> +Compatible cpu: "qcom,scorpion"
>>>> +Related properties: (none)
>>>
>>> From a look at the code, we need a node compatible with
>>> "qcom,gcc-msm8660" somewhere in the tree (see scss_release_secondary).
>>> It would be good to mention that.
>>
>> I will incorporate something here. I handled something similar
>> for qcom,kpss-acc-v? but missed this one I guess.
>
> It's easy to miss. It would be nice for the qcom guys to go over this
> and make sure that it says what they think it should.
>
>>
>>> [...]
>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/cpu-enable-method/spin-table.txt b/Documentation/devicetree/bindings/arm/cpu-enable-method/spin-table.txt
>>>> new file mode 100644
>>>> index 0000000..aee3617
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/arm/cpu-enable-method/spin-table.txt
>>>> @@ -0,0 +1,95 @@
>>>> +================================================
>>>> +Secondary CPU enable-method "spin-table" binding
>>>> +================================================
>>>> +
>>>> +This document describes the "spin-table" method for enabling secondary CPUs.
>>>> +A "spin-table" enable method can be used in either the "cpus" node or in
>>>> +individual "cpu" nodes.
>>>> +
>>>> +Enable method name: "spin-table"
>>>> +Compatible cpus: "arm,cortex-a57" (?)
>>>
>>> Any CPU with AArch64 may support this.
>>
>> Again, I based the document on the *.dts and *.dtsi files
>> present under arch/arm*/boot/dts, and in this case there
>> isn't one that uses "spin-table". The code existed though,
>> as did some documentation, so I included it here.
>>
>> I think what I'll do is parenthetically state what you said,
>> i.e., "(any CPU with AArch64)" in cases like this, so the
>> information is conveyed despite not having specific compatible
>> string values.
>
> Something like: "Compatible cpus: Any AArch64 CPU" would be fine. I'm
> not sure I see the point of the parens.

OK. The point was related to my attempt to put actual
compatible string values here, but I suppose the parentheses
don't really add value.

>>
>>>> +Related properties:
>>>> + - cpu-release-addr
>>>> + Usage: required
>>>> + Value type: <prop-encoded-array>
>>>
>>> Just say it's a 64-bit value, "prop-encoded-array" isn't all that
>>> helpful.
>>
>> Agreed. I was just copying what was there already.
>> I will fix it.
>
> Sure. Fitting in makes sense.
>
> A lot of the documentation we have is really vague, and
> "prop-encoded-array" is a particularly bad example of a description that
> provides no information whatsoever.
>
>>
>>>> + Definition:
>>>> + A two cell value identifying a 64-bit memory location
>>>> + used by the boot CPU to inform a secondary CPU it
>>>> + should begin its kernel bootstrap. Memory at this
>>>> + location must initially be zeroed.
>>>> +
>>>> +Examples (contrived 4-core ARM Cortex-A57 64-bit systems):
>>>> +
>>>> +The first example uses the same enable method for all cores.
>>>> +
>>>> + cpus {
>>>> + #size-cells = <0>;
>>>> + #address-cells = <2>;
>>>> + enable-method = "spin-table";
>>>> + cpu-release-addr = <0 0x20000000>;
>>>
>>> Linux currently expects this to be described per-cpu, and does not
>>> support either of these properties this being defined in /cpus for
>>> arm64.
>>
>> I'll take a closer look at the code and will update this.
>> I would much rather use a *real* (or at least close)
>> example. I made this out of whole cloth.
>
> You could take a look at arch/arm64/boot/dts/rtsm_ve-aemv8a.dts, and
> just munge the cpu-release-addr values to be unique.

That's great, thank you for pointing that out.

>>
>>> I would also very much like to discourage using a single release address
>>> -- it means we can only throw all CPUs into the kernel pen, where some
>>> may have to sit forever (breaking things like kexec). Ideally if a
>>> system has to use spin-table the addresses should be unique.
>>
>> That's fine with me. I don't have the details of
>> how this is supposed to work...
>
> Sure. I'd be happy to add a description as to the problems as a
> follow-up.

This is good, I appreciate your help.

-Alex


>>> So could we just have the example with unique addresses please?
>>
>> No problem, I'll create a new one; I hope you'll
>> verify that what I come up with is sane.
>
> Will do.
>
>>
>>> [...]
>>>
>>> Otherwise, this looks like a good first step. It would be nice to see
>>> some qcom folk review the qcom documentation changes.
>>
>> Yes it would. Thank you very much for looking this over
>> and providing feedback Mark. I'll have a new version out
>> later today.
>
> Cool. Thanks for putting this together, and apologies for the long delay
> on review.
>
> Cheers,
> Mark.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/