Date:	Tue, 03 Jun 2014 16:06:48 +0100
From:	Sudeep Holla <sudeep.holla@....com>
To:	Jassi Brar <jaswinder.singh@...aro.org>
CC:	Sudeep Holla <sudeep.holla@....com>,
	Jassi Brar <jassisinghbrar@...il.com>,
	Matt Porter <mporter@...aro.org>,
	Arnd Bergmann <arnd@...db.de>,
	lkml <linux-kernel@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Anna, Suman" <s-anna@...com>,
	Loic Pallardy <loic.pallardy@...com>,
	LeyFoon Tan <lftan.linux@...il.com>,
	Craig McGeachie <slapdau@...oo.com.au>,
	Courtney Cavin <courtney.cavin@...ymobile.com>,
	Rob Herring <robherring2@...il.com>,
	Josh Cartwright <joshc@...eaurora.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Kumar Gala <galak@...eaurora.org>,
	"ks.giri@...sung.com" <ks.giri@...sung.com>
Subject: Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

Hi Jassi,

On 03/06/14 11:21, Jassi Brar wrote:
> On 3 June 2014 15:05, Sudeep Holla <sudeep.holla@....com> wrote:
>> Hi Jassi,
>>
>> On Mon, Jun 2, 2014 at 6:11 PM, Jassi Brar <jassisinghbrar@...il.com> wrote:
>>> On Mon, Jun 2, 2014 at 8:44 PM, Matt Porter <mporter@...aro.org> 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@...dbeef {
>>>>          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@...dbeef {
>>>>          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 */
>>>> };

I am assuming this is what you referring below: cpm(which is the controller)
and cpm_ipc(a virtual node referring to the client). If yes that's fine, but is
this virtual node acceptable ?

>>>> ---
>>>>
>>>> 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@...dbeef {
>>>           compatible = "brcm,bcm-cpm-ipc";
>>>         /*  reg = <0xdeadbeef 0x100>; */
>>>         /*  interrupts = <0 123 4>;  */
>>>           mbox = <&cpm CPM_SYSTEM_CHANNEL>;
>>>           mbox-names = "system";
>>> };
>>>

Sorry got carried away with this example, assumed it to be accidental comment
and the proposal was to unify both controller and client. Now its clear, ignore
my noise earlier.

>>> 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.
>

Yes while I agree with you, but will it be acceptable for a generic mailbox
binding ? I think this is what even Matt mentioned earlier.

>> 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.
>

Ok that's good. I tried to follow previous discussions, but couldn't. I followed
only your last postings(v4) after I started using this driver.

>> 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.

Yes I understand that. So I was worried about unifying both controller and
protocol in one node. The controller driver must just handle raw data and never
interpret it. Now we just need DT maintainers' view on this virtual DT node.

Regards,
Sudeep

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

Powered by blists - more mailing lists