Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Adding devicetree list]

On Mon, Jun 02, 2014 at 10:41:44PM +0530, Jassi Brar wrote:

On Mon, Jun 2, 2014 at 8:44 PM, Matt Porter [off-list ref] wrote:

quoted

On Fri, May 30, 2014 at 11:01:55AM +0530, Jassi Brar wrote:

quoted

 Being more specific to your platform, I think you need some server
code (mailbox's client) that every driver (like clock, pmu, pinmux
etc) registers with to send messages to remote and receive commands
from remote ... perhaps by registering some filter to sort out
messages for each driver.

Right, and here's where you hit on the problem. This server you mention
is not a piece of hardware, it would be a software construct. As such, it
doesn't fit into the DT binding as it exists. It's probably best to
illustrate in DT syntax.

If I were to represent the hardware relationship in the DT binding now
it would look like this:

---
cpm: mailbox@deadbeef {
        compatible = "brcm,bcm-cpm-mailbox";
        reg = <...>;
        #mbox-cells <1>;
        interrupts = <...>;
};

/* clock complex */
ccu {
        compatible = "brcm,bcm-foo-ccu";
        mbox = <&cpm CPM_SYSTEM_CHANNEL>;
        mbox-names = "system";
        /* leaving out other mailboxes for brevity */
        #clock-cells <1>;
        clock-output-names = "bar",
                             "baz";
};

pmu {
        compatible = "brcm,bcm-foo-pmu"
        mbox = <&cpm CPM_SYSTEM_CHANNEL>;
        mbox-names = "system";
};

pinmux {
        compatible = "brcm,bcm-foo-pinctrl";
        mbox = <&cpm CPM_SYSTEM_CHANNEL>;
        mbox-names = "system";
};
---

Yeah, I too don't think its a good idea.

quoted

What we would need to do is completely ignore this information in each
of the of the client drivers associated with the clock, pmu, and pinmux
devices. This IPC server would need to be instantiated and get the
mailbox information from some source. mbox_request_channel() only works
when the client has an of_node with the mbox-names property present.
Let's say we follow this model and represent it in DT:

--
cpm: mailbox@deadbeef {
        compatible = "brcm,bcm-cpm-mailbox";
        reg = <...>;
        #mbox-cells <1>;
        interrupts = <...>;
};

cpm_ipc {
        compatible = "brcm,bcm-cpm-ipc";
        mbox = <&cpm CPM_SYSTEM_CHANNEL>;
        mbox-names = "system";
        /* leaving out other mailboxes for brevity */
};
---

This would allow an ipc driver to exclusively own this system channel,
but now we've invented a binding that doesn't reflect the hardware at
all. It's describing software so I don't believe the DT maintainers will
allow this type of construct.

Must the server node specify MMIO and an IRQ, to be acceptable? Like ...

My bad, that was a cut and paste typo..I intended to remove those
properties as you do below.

cpm_ipc : cpm@deadbeef {
         compatible = "brcm,bcm-cpm-ipc";
       /*  reg = <0xdeadbeef 0x100>; */
       /*  interrupts = <0 123 4>;  */
         mbox = <&cpm CPM_SYSTEM_CHANNEL>;
         mbox-names = "system";
};

Correct, this should have been:

cpm_ipc {
	compatible = "brcm,bcm-cpm-ipc";
	mbox = <&cpm CPM_SYSTEM_CHANNEL>;
	mbox-names = "system";
};

cpm_ipc already specifies a hardware resource (mbox) that its driver
needs, I think that should be enough reason. If it were some purely
soft property for the driver like
      mode = "poll";  //or "irq"
then the node wouldn't be justified because that is the job of a
build-time config or run-time module option.

Hrm, this is where I'd like to hear from the DT maintainers since we
have to live with this generic binding. If they are ok with us creating
bindings for a virtual device that exists only to match with our ipc
driver then it will work. I'm not aware of other similar examples though.

-Matt
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html