Re: [PATCH v2 2/9] dt-bindings: x86: Add a binding for x86 wakeup mailbox

[Yunhong Jiang <yunhong.jiang@linux.intel.com>]

On Thu, Sep 19, 2024 at 12:19:01PM -0700, Yunhong Jiang wrote:
> On Mon, Sep 16, 2024 at 10:56:38AM +0200, Krzysztof Kozlowski wrote:
> > On 10/09/2024 08:13, Yunhong Jiang wrote:
> > > On Tue, Aug 27, 2024 at 01:45:49PM -0700, Yunhong Jiang wrote:
> > >> On Sun, Aug 25, 2024 at 09:10:01AM +0200, Krzysztof Kozlowski wrote:
> > >>> On Fri, Aug 23, 2024 at 04:23:20PM -0700, Yunhong Jiang wrote:
> > >>>> Add the binding to use mailbox wakeup mechanism to bringup APs.
> > >>>>
> > >>>> Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > >>>> ---
> > >>>>  .../devicetree/bindings/x86/wakeup.yaml       | 64 +++++++++++++++++++
> > >>>>  1 file changed, 64 insertions(+)
> > >>>>  create mode 100644 Documentation/devicetree/bindings/x86/wakeup.yaml
> > >>>>
> > >>>> diff --git a/Documentation/devicetree/bindings/x86/wakeup.yaml b/Documentation/devicetree/bindings/x86/wakeup.yaml
> > >>>> new file mode 100644
> > >>>> index 000000000000..cb84e2756bca
> > >>>> --- /dev/null
> > >>>> +++ b/Documentation/devicetree/bindings/x86/wakeup.yaml
> > >>>> @@ -0,0 +1,64 @@
> > >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > >>>> +# Copyright (C) 2024 Intel Corporation
> > >>>> +%YAML 1.2
> > >>>> +---
> > >>>> +$id: http://devicetree.org/schemas/x86/wakeup.yaml#
> > >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >>>> +
> > >>>> +title: x86 mailbox wakeup
> > >>>> +maintainers:
> > >>>> +  - Yunhong Jiang <yunhong.jiang@linux.intel.com>
> > >>>> +
> > >>>> +description: |
> > >>>> +  The x86 mailbox wakeup mechanism defines a mechanism to let the bootstrap
> > >>>> +  processor (BSP) to wake up application processors (APs) through a wakeup
> > >>>> +  mailbox.
> > >>>> +
> > >>>> +  The "wakeup-mailbox-addr" property specifies the wakeup mailbox address. The
> > >>>> +  wakeup mailbox is a 4K-aligned 4K-size memory block allocated in the reserved
> > >>>> +  memory.
> > >>>> +
> > >>>> +  The wakeup mailbox structure is defined as follows.
> > >>>> +
> > >>>> +    uint16_t command;
> > >>>> +    uint16_t reserved;
> > >>>> +    uint32_t apic_id;
> > >>>> +    uint64_t wakeup_vector;
> > >>>> +    uint8_t  reservedForOs[2032];
> > >>>> +
> > >>>> +  The memory after reservedForOs field is reserved and OS should not touch it.
> > >>>> +
> > >>>> +  To wakes up a AP, the BSP prepares the wakeup routine, fills the wakeup
> > >>>> +  routine's address into the wakeup_vector field, fill the apic_id field with
> > >>>> +  the target AP's APIC_ID, and write 1 to the command field. After receiving the
> > >>>> +  wakeup command, the target AP will jump to the wakeup routine.
> > >>>> +
> > >>>> +  For each AP, the mailbox can be used only once for the wakeup command. After
> > >>>> +  the AP jumps to the wakeup routine, the mailbox will no longer be checked by
> > >>>> +  this AP.
> > >>>> +
> > >>>> +  The wakeup mailbox structure and the wakeup process is the same as
> > >>>> +  the Multiprocessor Wakeup Mailbox Structure defined in ACPI spec version 6.5,
> > >>>> +  section 5.2.12.19 [1].
> > >>>> +
> > >>>> +  References:
> > >>>> +
> > >>>> +  [1] https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html
> > >>>> +
> > >>>> +select: false
> > >>>
> > >>> This schema is still a no-op because of this false.
> > >>>
> > >>> What is the point of defining one property if it is not placed anywhere?
> > >>> Every device node can have it? Seems wrong...
> > >>>
> > >>> You need to come with proper schema. Lack of an example is another thing
> > >>> - this cannot be even validated by the tools. 
> > >>>
> > >>> Best regards,
> > >>> Krzysztof
> > > 
> > > Hi, Krzysztof, I'm working to address your comments and have some questions.
> > > Hope to get help/guide from your side.
> > > 
> > > For the select, the writing-schema.rst describes it as "A json-schema used to
> > > match nodes for applying the schema" but I'm a bit confused. In my case, should
> > > it be "cpus" node? Is there any code/tools that uses this property, so that I
> > > can have a better understanding?
> > 
> > Usually we expect matching by compatible, but it does not seem suitable
> > here because it is not related to any specific device, right? That is
> > the problem with all this DT-reuse-for-virtual-stuff work. It just does
> > not follow usual expectations and guidelines - you do not describe a device.
> 
> Thank you for the reply.
> 
> I'm a bit confused on your "do not describe a device".
> I think VM is also a device, it's just a virtual device, but I don't see much
> difference of the virtual and physical device from DT point of view, possibly I
> missed some point.
>  
> > 
> > You can still match by nodes. See all top-level bindings.
> 
> After checking the code at
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/validator.py,
> seems the 'select' is translated to 'if'/'then'.
> 
> Do you have any example of "top-level bindings"? I tried to check binding for
> enable-methods like arm/cpu-enable-method/nuvoton,npcm750-smp or
> cpu/idle-states.yaml, but they are either not schema file, or quite different.
> 
> I have been struggling on this device binding document for a while. I
> reconsidered what this binding is for. This binding means, if the cpus node has
> "enable-method" as "acpi-wakeup-mailbox", then the device should have property
> "wakeup-mailbox-addr" with uint64 type.
> 
> In that case, I'm considering to set the "select" to be true so that it will
> apply to any potential device, and add if/then keyword to check the
> enable-method. But seems it does not work and I'm still trying to figure out the
> reason (I'm new to the json/json schema and is still learning).
> 
> I received followed error:
> cpus: '#address-cells', '#size-cells', 'cpu@0', 'enable-method' do not match any of the regexes: 'pinctrl-[0-9]+'
>         from schema $id: http://devicetree.org/schemas/x86/wakeup.yaml#
> cpu@0: 'device_type', 'reg' do not match any of the regexes: 'pinctrl-[0-9]+'
>         from schema $id: http://devicetree.org/schemas/x86/wakeup.yaml#
> 
> With the followed yaml file (I delete some description).
> 
> $ cat Documentation/devicetree/bindings/x86/wakeup.yaml
> # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> # Copyright (C) 2024 Intel Corporation
> %YAML 1.2
> ---
> $id: http://devicetree.org/schemas/x86/wakeup.yaml#
> $schema: http://devicetree.org/meta-schemas/core.yaml#
> 
> title: x86 mailbox wakeup
> maintainers:
>   - Yunhong Jiang <yunhong.jiang@linux.intel.com>
> 
> description: |
>   ......
>   Removed to save space.
> 
> properties:
>   wakeup-mailbox-addr:
>     $ref: /schemas/types.yaml#/definitions/uint32
>     description: |
>       ......
>       Removed to save space.
> 
> select: true
> 
> if:
>   properties:
>     enable-method:
>       contains:
>         const: acpi-wakeup-mailbox
>   required:
>     - enable-method
> 
> then:
>   required:
>     - wakeup-mailbox-addr
> 
> additionalProperties: false
> 
> examples:
>   - |
>     cpus {
>       #address-cells = <1>;
>       #size-cells = <0>;
>       enable-method = "acpi-wakeup-mailbox";
>       wakeup-mailbox-addr = <0x1c000500>;
>       cpu@0 {
>         device_type = "cpu";
>         reg = <0x1>;
>       };
>     };
> ...
> 
> > 
> > > 
> > > For your "validated by the tools", can you please share the tools you used to
> > > validate the schema? I used "make dt_binding_check" per the
> > > submitting-patches.rst but I think your comments is about another tool.
> > 
> > See writing-schema document.
> Yes, I figured out in the end that the validate tools means the dt-schema tools.
> 
> Thank you
> --jyh

One thing just come to my mind. The
Documentation/devicetree/bindings/x86/wakeup.yaml only adds one property,
wakeup-mailbox-addr, to the cpus node, and it does not specify any new object.
Per my understanding, the json schema file normally specifies a new object.
Is my change a supported scenario in current Documentation/devicetree?

I checked similar scenario like arm/cpu-enable-method/nuvoton and
npcm750-smp/cpu-enable-method/al,alpine-smp, they don't present as a json
schema file. Wonder if it's because the same reason.

> 
> > 
> > 
> > Best regards,
> > Krzysztof
> > 
> > 
>