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

[Jassi Brar]

On 3 June 2014 15:05, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
> Hi Jassi,
>
> On Mon, Jun 2, 2014 at 6:11 PM, Jassi Brar <jassisinghbrar@xxxxxxxxx> wrote:
>> On Mon, Jun 2, 2014 at 8:44 PM, Matt Porter <mporter@xxxxxxxxxx> wrote:
>>> On Fri, May 30, 2014 at 11:01:55AM +0530, Jassi Brar wrote:
>>>
>>>>  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.
>>
>>
>>> 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 ...
>>
>> cpm_ipc : cpm@deadbeef {
>>          compatible = "brcm,bcm-cpm-ipc";
>>        /*  reg = <0xdeadbeef 0x100>; */
>>        /*  interrupts = <0 123 4>;  */
>>          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.
>>
>
> Like Matt, I am also in similar situation where there's a lot of common
> code necessary to construct/parse IPCs for each of the drivers using the
> mailbox.
>
> As per your suggestion if we have single DT node to specify both the
> controller and the client, we might still have to pollute this node with
> software specific compatibles.
>
I am afraid you misunderstood me. I don't suggest single node for
mailbox controller and client, and IIUC, neither did Matt. Please note
the controller is cpm and client is cpm_ipc.

BTW, here we at least have a hardware resource to specify in the DT
node, there are examples in kernel where the DT nodes are purely
virtual. For ex, grep for "linux,spdif-dit". So I think we should be
ok.

> One possible scenario I can think of is that if the mailbox controller is
> a standard primecell like PL320 used in multiple SoCs, each SoC vendor
> will develop their own protocol implemented in their firmware. This is true
> even with single SoC vendor having same IP but changing the protocol to
> talk to their firmware.
>
Yeah, people have noted that in previous threads.

> We will need a way to identify that protocol mechanism.
> Does it make sense to add that  ? Is that something acceptable ?
>
 IMO we can't help it more than _trying_ to write the controller
driver as versatile as possible. And still some protocol
version/peculiarity could make reuse of the controller driver worse
than write a new for the protocol version. Any minor change in
behavior could be flagged to controller and client in platform
specific way.

Regards,
-Jassi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/