Re: [PATCH v3 06/13] dt-bindings: reserved-memory: Wakeup Mailbox for Intel processors

[Rob Herring <robh@kernel.org>]

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.


> > 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, 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? 

Rob