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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 03/03/2025 23:21, Ricardo Neri wrote:
> On Fri, Sep 20, 2024 at 01:15:41PM +0200, Krzysztof Kozlowski wrote:
> 
> [...]
>  
>> enable-method is part of CPUs, so you probably should match the CPUs...
>> I am not sure, I don't have the big picture here.
>>
>> Maybe if companies want to push more of bindings for purely virtual
>> systems, then they should first get involved more, instead of relying on
>> us. Provide reviews for your virtual stuff, provide guidance. There is
>> resistance in accepting bindings for such cases for a reason - I don't
>> even know what exactly is this and judging/reviewing based on my
>> practices will no be accurate.
> 
> Hi Krzysztof,
> 
> I am taking over this work from Yunhong.
> 
> First of all, I apologize for the late reply. I will make sure
> communications are timely in the future.
> 
> Our goal is to describe in the device tree a mechanism or artifact to boot
> secondary CPUs.
> 
> In our setup, the firmware puts secondary CPUs to monitor a memory location
> (i.e., the wakeup mailbox) while spinning. From the boot CPU, the OS writes
> in the mailbox the wakeup vector and the ID of the secondary CPU it wants
> to boot. When a secondary CPU sees its own ID it will jump to the wakeup
> vector.
> 
> This is similar to the spin-table described in the Device Tree
> specification. The key difference is that with the spin-table CPUs spin
> until a non-zero value is written in `cpu-release-addr`. The wakeup mailbox
> uses CPU IDs.
> 
> You raised the issue of the lack of a `compatible` property, and the fact
> that we are not describing an actual device.
> 
> I took your suggestion of matching by node and I came up with the binding
> below. I see these advantages in this approach:
> 
>   * I define a new node with a `compatible` property.
>   * There is precedent: the psci node. In the `cpus` node, each cpu@n has

psci is a standard. If you are documenting here a standard, clearly
express it and provide reference to the specification.


>     an `enable-method` property that specify `psci`.
>   * The mailbox is a device as it is located in a reserved memory region.
>     This true regardless of the device tree describing bare-metal or
>     virtualized machines.
> 
> Thanks in advance for your feedback!
> 
> Best,
> Ricardo
> 
> (only the relevant sections of the binding are shown for brevity)
> 
> properties:
>   $nodename:
>     const: wakeup-mailbox
> 
>   compatible:
>     const: x86,wakeup-mailbox

You need vendor prefix for this particular device. If I pointed out lack
of device and specific compatible, then adding random compatible does
not solve it. I understand it solves for you, but not from the bindings
point of view.

> 
>   mailbox-addr:
>     $ref: /schemas/types.yaml#/definitions/uint64

So is this some sort of reserved memory? Mailbox needs mbox-cells, so
maybe that's not mailbox.



Best regards,
Krzysztof