On Sat, May 24, 2025 at 08:56:50AM -0700, Ricardo Neri wrote:
> On Mon, May 19, 2025 at 10:56:06AM -0700, Ricardo Neri wrote:
> > On Mon, May 19, 2025 at 10:29:37AM -0500, Rob Herring wrote:
> > > On Wed, May 14, 2025 at 08:53:38PM -0700, Ricardo Neri wrote:
> > > > On Wed, May 14, 2025 at 10:42:48AM -0500, Rob Herring wrote:
> > > > > On Tue, May 13, 2025 at 03:14:56PM -0700, Ricardo Neri wrote:
> > > > > > On Mon, May 12, 2025 at 10:32:24AM -0500, Rob Herring wrote:
> > > > > > > On Tue, May 06, 2025 at 08:23:39PM -0700, Ricardo Neri wrote:
> > > > > > > > On Tue, May 06, 2025 at 09:10:22AM +0200, Krzysztof Kozlowski
> > > > > > > > wrote:
> > > > > > > > > On Mon, May 05, 2025 at 10:16:10PM GMT, Ricardo Neri wrote:
> > > > > > > > > > > If this is a device, then compatibles specific to
> > > > > > > > > > > devices. You do not
> > > > > > > > > > > get different rules than all other bindings... or this
> > > > > > > > > > > does not have to
> > > > > > > > > > > be binding at all. Why standard reserved-memory does not
> > > > > > > > > > > work for here?
> > > > > > > > > > >
> > > > > > > > > > > Why do you need compatible in the first place?
> > > > > > > > > >
> > > > > > > > > > Are you suggesting something like this?
> > > > > > > > > >
> > > > > > > > > > reserved-memory {
> > > > > > > > > > # address-cells = <2>;
> > > > > > > > > > # size-cells = <1>;
> > > > > > > > > >
> > > > > > > > > > wakeup_mailbox: wakeupmb@fff000 {
> > > > > > > > > > reg = < 0x0 0xfff000 0x1000>
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > and then reference to the reserved memory using the
> > > > > > > > > > wakeup_mailbox
> > > > > > > > > > phandle?
> > > > > > > > >
> > > > > > > > > Yes just like every other, typical reserved memory block.
> > > > > > > >
> > > > > > > > Thanks! I will take this approach and drop this patch.
> > > > > > >
> > > > > > > If there is nothing else to this other than the reserved region,
> > > > > > > then
> > > > > > > don't do this. Keep it like you had. There's no need for 2 nodes.
> > > > > >
> > > > > > Thank you for your feedback!
> > > > > >
> > > > > > I was planning to use one reserved-memory node and inside of it a
> > > > > > child
> > > > > > node to with a `reg` property to specify the location and size of
> > > > > > the
> > > > > > mailbox. I would reference to that subnode from the kernel code.
> > > > > >
> > > > > > IIUC, the reserved-memory node is only the container and the actual
> > > > > > memory
> > > > > > regions are expressed as child nodes.
> > > > > >
> > > > > > I had it like that before, but with a `compatible` property that I
> > > > > > did not
> > > > > > need.
> > > > > >
> > > > > > Am I missing anything?
> > > > >
> > > > > Without a compatible, how do you identify which reserved region is
> > > > > the
> > > > > wakeup mailbox?
> > > >
> > > > I thought using a phandle to the wakeup_mailbox. Then I realized that
> > > > the
> > > > device nodes using the mailbox would be CPUs. They would need a `memory-
> > > > region` property. This does not look right to me.
> > >
> > > That doesn't really make sense unless it's a memory region per CPU.
> >
> > Agreed.
> >
> > >
> > >
> > > > > Before you say node name, those are supposed to be
> > > > > generic though we failed to enforce anything for /reserved-memory
> > > > > child
> > > > > nodes.
> > > >
> > > > I see. Thanks for preventing me from doing this.
> > > >
> > > > Then the `compatible` property seems the way to go after all.
> > > >
> > > > This what motivated this patch in the first place. On further analysis,
> > > > IIUC, defining bindings and schema is not needed, IMO, since the mailbox
> > > > is already defined in the ACPI spec. No need to redefine.
> > >
> > > You lost me...
> > >
> > > You don't need to redefine the layout of the memory region as that's
> > > defined already somewhere,
> >
> > Great!
> >
> > > but you do need to define where it is for DT.
> > > And for that, you need a compatible. Do you know where it is in this
> > > case?
> >
> > The compatible is not defined anywhere yet. Is a DT schema needed to
> > document it? If yes, I am usure what to put in the description. We tried
> > to not redefine the mailbox and refer to the ACPI spec. That was a NAK
> > from Krzysztof [1].
> >
> > [1].
> > https://lore.kernel.org/r/624e1985-7dd2-4abe-a918-78cb43556...@kernel.org
>
> In summary, documenting the `compatible` property for the mailbox is
> necessary. There is no need to redefine the malbox on a schema but
> referring to the ACPI spec is not acceptable.
There's the whole "DT bindings in ACPI systems" where ACPI tables
contain compatibles and DT properties which I think is what
Krzysztof was objecting to (and I do too). But this is a DT based system
that implements a mailbox region defined in an ACPI spec. That is
perfectly fine to refer to.
>
> What about referring in the schema to the Intel TDX Virtual Firmware Design
> Guide[2]? It describes how firmware should implement the mailbox the section
> 4.3.5.
>
> A mailbox with compatible = "intel,wakeup-mailbox" is implemented after the
> guide that Intel published.
Use whatever you think best describes the programming model of the
region.
Rob
