Message-ID: <20250506-alluring-beaver-of-modernism-65ff8a@kuoka>
Date: Tue, 6 May 2025 09:25:59 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
Cc: x86@...nel.org, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Rob Herring <robh@...nel.org>, 
	"K. Y. Srinivasan" <kys@...rosoft.com>, Haiyang Zhang <haiyangz@...rosoft.com>, 
	Wei Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>, 
	Michael Kelley <mhklinux@...look.com>, devicetree@...r.kernel.org, 
	Saurabh Sengar <ssengar@...ux.microsoft.com>, Chris Oo <cho@...rosoft.com>, linux-hyperv@...r.kernel.org, 
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>, linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org, 
	"Ravi V. Shankar" <ravi.v.shankar@...el.com>, Ricardo Neri <ricardo.neri@...el.com>
Subject: Re: [PATCH v3 04/13] dt-bindings: x86: Add CPU bindings for x86

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-calderon@...ux.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@...ux.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

Powered by blists - more mailing lists