On Mon, May 05, 2025 at 09:52:35PM GMT, Ricardo Neri wrote: > On Sun, May 04, 2025 at 06:45:59PM +0200, Krzysztof Kozlowski wrote: > > On Sat, May 03, 2025 at 12:15:06PM GMT, Ricardo Neri wrote: > > > Add bindings for CPUs in x86 architecture. Start by defining the `reg` and > > > > What for? > > Thank you for your quick feedback, Krzysztof! > > Do you mean for what reason I want to start bindings for x86 CPUs? Or only
Yes. For which devices, what purpose. > the `reg` property? If the former, it is to add an enable-method property to > x86 CPUs. If the latter, is to show the relationship between APIC and `reg`. > > > > > > `enable-method` properties and their relationship to x86 APIC ID and the > > > available mechanisms to boot secondary CPUs. > > > > > > Start defining bindings for Intel processors. Bindings for other vendors > > > can be added later as needed. > > > > > > Signed-off-by: Ricardo Neri <ricardo.neri-calde...@linux.intel.com> > > > --- > > > > Not really tested so only limited review follows. > > Sorry, I ran make dt_binding_check but only on this schema. I missed the > reported error. > > > > > > .../devicetree/bindings/x86/cpus.yaml | 80 +++++++++++++++++++ > > > 1 file changed, 80 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/x86/cpus.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/x86/cpus.yaml > > > b/Documentation/devicetree/bindings/x86/cpus.yaml > > > new file mode 100644 > > > index 000000000000..108b3ad64aea > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/x86/cpus.yaml > > > @@ -0,0 +1,80 @@ > > > +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/x86/cpus.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: x86 CPUs > > > + > > > +maintainers: > > > + - Ricardo Neri <ricardo.neri-calde...@linux.intel.com> > > > + > > > +description: | > > > + Description of x86 CPUs in a system through the "cpus" node. > > > + > > > + Detailed information about the CPU architecture can be found in the > > > Intel > > > + Software Developer's Manual: > > > + https://intel.com/sdm > > > + > > > +properties: > > > + compatible: > > > + enum: > > > + - intel,x86 > > > > That's architecture, not a CPU. CPUs are like 80286, 80386, so that's > > not even specific instruction set. I don't get what you need it for. > > Am I to understand the the `compatible` property is not needed if the > bindings apply to any x86 CPU? Every device needs compatible. Its meaning is explained: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#compatible If you add here a device representing CPU, then look at existing bindings for CPUs how they do it. It again feels like you add DT for platform which is not a real thing. If you use DT, you do not get different rules, therefore read all standard guides and tutorials (there were many, quite comprehensive). > > > > > > + > > > + reg: > > > > Missing constraints. > > I could add minItems. For maxItems, there is no limit to the number of > threads. I am pretty sure that any given CPU, e.g. 80486 has a fixed number of threads... > > > > > > + description: | > > > > Do not need '|' unless you need to preserve formatting. > > OK. > > > > > > + Local APIC ID of the CPU. If the CPU has more than one execution > > > thread, > > > + then the property is an array with one element per thread. > > > + > > > + enable-method: > > > + $ref: /schemas/types.yaml#/definitions/string > > > + description: | > > > + The method used to wake up secondary CPUs. This property is not > > > needed if > > > + the secondary processors are booted using INIT assert, de-assert > > > followed > > > + by Start-Up IPI messages as described in the Volume 3, Section > > > 11.4 of > > > + Intel Software Developer's Manual. > > > + > > > + It is also optional for the bootstrap CPU. > > > + > > > + oneOf: > > > > I see only one entry, so didn't you want an enum? > > Indeed, enum would be more appropriate. > > > > > > + - items: > > > > Not a list > > > > > + - const: intel,wakeup-mailbox > > > > So every vendor is supposed to come with different name for the same > > feature? Or is wakeup-mailnox really intel specific, but then specific > > to which processors? > > It would not be necessary for every vendor to provide a different name for > the same feature. I saw, however that the Devicetree specification requires > a [vendor],[method] stringlist. Indeed, it's fine then. > > Also, platform firmware for any processor could implement the wakeup > mailbox. > > > > > > > > + description: | > > > + CPUs are woken up using the mailbox mechanism. The platform > > > + firmware boots the secondary CPUs and puts them in a state > > > + to check the mailbox for a wakeup command from the > > > operating > > > + system. > > > + > > > +required: > > > + - reg > > > + - compatible > > > + > > > +unevaluatedProperties: false > > > > Missing ref in top-level or this is supposed to be additionalProps. See > > example-schema. > > I will check. > > > > > > + > > > +examples: > > > + - | > > > + /* > > > + * A system with two CPUs. cpu@0 is the bootstrap CPU and its status > > > is > > > + * "okay". It does not have the enable-method property. cpu@1 is a > > > + * secondary CPU. Its status is "disabled" and defines the > > > enable-method > > > + * property. > > > + */ > > > + > > > + cpus { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + cpu@0 { > > > + reg = <0x0 0x1>; > > > + compatible = "intel,x86"; > > > + status = "okay"; > > > > Drop > > I will drop status = "okay" > > > > > > + }; > > > + > > > + cpu@1 { > > > + reg = <0x0 0x1>; > > > + compatible = "intel,x86"; > > > + status = "disabled"; > > > > Why? > > Because this is a secondary CPU that the operating system will enable using > the method specified in the `enable-method` property. OK, so this was intentional to express it is in quiescent state. Best regards, Krzysztof
