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

[Yunhong Jiang]

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@xxxxxxxxxxxxxxx>
> > > ---
> > >  .../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@xxxxxxxxxxxxxxx>
> > > +
> > > +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?

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.

Sorry for the bothering. I read the DT spec and the
Documentation/devicetree/bindings documents and still not sure.

Than you
--jyh

> 
> Thank you for the feedback. Will update the schema file on next round
> submission.
> 
> Thanks
> --jyh
> 
> > 
>