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

[Ricardo Neri <ricardo.neri-calderon@linux.intel.com>]

On Wed, Mar 19, 2025 at 08:52:37AM +0100, Krzysztof Kozlowski wrote:
> On 12/03/2025 06:51, Ricardo Neri wrote:
> > On Tue, Mar 11, 2025 at 11:01:28AM +0100, Krzysztof Kozlowski wrote:
> >> 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
> >>
> > 
> > Thanks for your feedback!
> > 
> >> psci is a standard. If you are documenting here a standard, clearly
> >> express it and provide reference to the specification.
> > 
> > It is not really a standard, but this mailbox behaves indentically to the
> > wakeup mailbox described in the ACPI spec [1]. I am happy reference the
> > spec in the documentation of the binding... or describe in full the
> > mechanism of mailbox without referring to ACPI. You had indicated you don't
> > care about what ACPI does [2].
> 
> Behaving like ACPI and implementing a spec are two different things. The
> question is whether you need to implement it like that and I believe
> answer is: no.
> 
> > 
> > In a nutshell, the wakeup mailbox is similar to the spin table used in ARM
> > boards: it is reserved memory region that secondary CPUs monitor while
> > spinning.
> > 
> >>
> >>
> >>>     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.
> > 
> > I see. Platform firmware will implement the mailbox. It would not be any
> > specific hardware from Intel. Perhaps `intel,wakeup-mailbox`?
> > 
> >>
> >>>
> >>>   mailbox-addr:
> >>>     $ref: /schemas/types.yaml#/definitions/uint64
> >>
> >> So is this some sort of reserved memory?
> > 
> > Yes, the mailbox is located in reserved memory.
> 
> 
> Then why reserved memory bindings are not working?
> 
> Anyway this was half a year ago. None of the emails are in my inbox.
> None of the context is in my head.
> 
> It's the second or third email this month someone responds to my email
> from 2024.
> 
> Frankly, that's a neat trick. I won't remember anything, but it would be
> impolite to say just "no" without arguments. So now you will resend the
> same code leading to the same discussions we had half a year ago. Or
> ignoring that discussions.
> 
> I don't understand why this should be reviewers problem, so no, that's
> just unfair.
> 

Thank you again for your feedback. I will send a third version of the
patchset. This will give the full context. I will incorporate your feedback
in such version.

Thanks and BR,
Ricardo