Re: [PATCH v3 04/13] dt-bindings: x86: Add CPU bindings for x86

Ricardo Neri
2025-05-06T04:47:57Z

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
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-calderon@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-calderon@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?

>
> > +
> > + reg:
>
> Missing constraints.

I could add minItems. For maxItems, there is no limit to the 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.

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.

Thanks and BR,
Ricardo