On Fri, May 30, 2014 at 11:01:55AM +0530, Jassi Brar wrote:
> On Thu, May 29, 2014 at 9:13 PM, Matt Porter <[email protected]> wrote:
> > On Fri, May 16, 2014 at 07:03:25PM +0530, Jassi Brar wrote:
> >> On 15 May 2014 19:57, Arnd Bergmann <[email protected]> wrote:
> >> > On Thursday 15 May 2014 11:41:00 Jassi Brar wrote:
> >
> > ...
> >
> >> >> +struct mbox_controller {
> >> >> + struct device *dev;
> >> >> + struct mbox_chan_ops *ops;
> >> >> + struct mbox_chan *chans;
> >> >> + int num_chans;
> >> >> + bool txdone_irq;
> >> >> + bool txdone_poll;
> >> >> + unsigned txpoll_period;
> >> >> + struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox,
> >> >> + const struct of_phandle_args *sp);
> >> >> + /*
> >> >> + * If the controller supports only TXDONE_BY_POLL,
> >> >> + * this timer polls all the links for txdone.
> >> >> + */
> >> >> + struct timer_list poll;
> >> >> + unsigned period;
> >> >> + /* Hook to add to the global controller list */
> >> >> + struct list_head node;
> >> >> +} __aligned(32);
> >> >
> >> > What is the __aligned(32) for?
> >> >
> >> Attempt to align access to mailbox?
> >>
> >> I am still open to opinion about whether the mailbox ownership should
> >> be exclusive or shared among clients. Need to handle async messages
> >> from remote is one reason one might want more than one client to own a
> >> channel. Allowing for RX via notifiers might be one option but that
> >> breaks semantics of 'ownership' of a mailbox channel.
> >
> > This is the case we have on a new family of Broadcom SoCs. One mailbox
> > channel is the "system" channel and is shared amongst many clients for
> > reception of unsolicited events from the coprocessor. The same channel
> > is also used commonly in drivers for debug/inspection of the M0 state.
> > Functionality for clock, pmu, pinmux, and cpu idle/suspend is implemented
> > via IPC with the M0 so all of those drivers need to share one mailbox.
> >
> OK, so you have a single mailbox channel used for communication with
> the remote master.
Yes, specifically, single mailbox channel is shared by many clients
for interrupt events.
>
> > There's a lot of common code necessary to construct/parse IPCs for
> > each of the drivers. I suppose this IPC driver/api could be the
> > sole owner of that system channel. However, we'd need to develop some
> > binding to represent IPC resources that devices need to operate. Coming
> > into this late...I wonder if I missed something about where these vendor
> > specific IPC layers should live and how, if it makes sense, they should
> > be represented in DT. In our case, the IPC is an integral part of the
> > hardware as it's loaded from ROM.
> >
> Like other platforms, the IPC protocol is going to be very specific to
> Broadcom, even if the mailbox controller is some popular third party
> IP like PL320. So you/Broadcom have to implement parser code for IPC
> (client) above the mailbox common api and the controller driver below
> it. Any resource/feature specific to your client and your controller
> should be passed via some Broadcom specific DT bindings. The common
> mailbox api DT bindings provide only for channel-client mapping.
Agreed.
> 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";
};
---
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.
One possibly is to bring back string-based channel matching, and allow
the controller node to optionally handle the mbox-names property. As in:
---
cpm: mailbox@deadbeef {
compatible = "brcm,bcm-cpm-mailbox";
reg = <...>;
interrupts = <...>;
#mbox-cells <1>;
mbox-names = "foo",
"bar",
"baz",
"system";
};
---
and allow a non-DT based mbox_request_channel() path using string
matching. I know it's icky and the reasons for dropping that in the
first place but I'm just throwing out one option that would work here.
-Matt
On Mon, Jun 2, 2014 at 8:44 PM, Matt Porter <[email protected]> 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.
Regards,
-Jassi
[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 <[email protected]> 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 ...
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
Hi Jassi,
On Mon, Jun 2, 2014 at 6:11 PM, Jassi Brar <[email protected]> wrote:
> On Mon, Jun 2, 2014 at 8:44 PM, Matt Porter <[email protected]> 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.
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. We will need a way to identify that protocol mechanism.
Does it make sense to add that ? Is that something acceptable ?
Regards,
Sudeep
On 3 June 2014 15:05, Sudeep Holla <[email protected]> wrote:
> Hi Jassi,
>
> On Mon, Jun 2, 2014 at 6:11 PM, Jassi Brar <[email protected]> wrote:
>> On Mon, Jun 2, 2014 at 8:44 PM, Matt Porter <[email protected]> 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
Hi Jassi,
On 03/06/14 11:21, Jassi Brar wrote:
> On 3 June 2014 15:05, Sudeep Holla <[email protected]> wrote:
>> Hi Jassi,
>>
>> On Mon, Jun 2, 2014 at 6:11 PM, Jassi Brar <[email protected]> wrote:
>>> On Mon, Jun 2, 2014 at 8:44 PM, Matt Porter <[email protected]> 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 */
>>>> };
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@deadbeef {
>>> 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
On Tue, Jun 03, 2014 at 03:51:55PM +0530, Jassi Brar wrote:
> On 3 June 2014 15:05, Sudeep Holla <[email protected]> wrote:
> > Hi Jassi,
> >
> > On Mon, Jun 2, 2014 at 6:11 PM, Jassi Brar <[email protected]> wrote:
> >> On Mon, Jun 2, 2014 at 8:44 PM, Matt Porter <[email protected]> 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.
Correct, I had separate controller and consumer nodes as written
above...to match the binding.
> 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.
>
There's a bit of a difference between my concern over a virtual node and
this example you've cited. In the dummy spdif transmitter, it's defining
a virtual device that plugs in for a codec, a hardware concept well
defined in the audio bindings. I agree that there are many examples of
this type of virtual node, including dummy phys, but in all cases they
are stubbing out a real hardware concept.
I find it to be distinctly different to create a node that doesn't
represent the hardware's use of mailboxes. I'd be happy if a DT
maintainer could say that this is acceptable though. ;)
-Matt
On 5 June 2014 16:42, Matt Porter <[email protected]> wrote:
> On Tue, Jun 03, 2014 at 03:51:55PM +0530, Jassi Brar wrote:
>
>> 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.
>>
>
> There's a bit of a difference between my concern over a virtual node and
> this example you've cited. In the dummy spdif transmitter, it's defining
> a virtual device that plugs in for a codec, a hardware concept well
> defined in the audio bindings. I agree that there are many examples of
> this type of virtual node, including dummy phys, but in all cases they
> are stubbing out a real hardware concept.
>
> I find it to be distinctly different to create a node that doesn't
> represent the hardware's use of mailboxes.
>
The way I see "cpm_ipc" is that it represents a device that doesn't
need MMIO or an IRQ, but only the mailbox hardware resource.
"linux,spdif-dit" needs no hardware resource at all. So if anything,
more "virtual" than cpm_ipc.
> I'd be happy if a DT
> maintainer could say that this is acceptable though. ;)
>
OK, though it becomes clear only after reading this ;)
Cheers
-Jassi
On Thu, Jun 05, 2014 at 07:12:05AM -0400, Matt Porter wrote:
> On Tue, Jun 03, 2014 at 03:51:55PM +0530, Jassi Brar wrote:
> > 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.
> There's a bit of a difference between my concern over a virtual node and
> this example you've cited. In the dummy spdif transmitter, it's defining
> a virtual device that plugs in for a codec, a hardware concept well
> defined in the audio bindings. I agree that there are many examples of
> this type of virtual node, including dummy phys, but in all cases they
> are stubbing out a real hardware concept.
Well, really it's an actual physical thing - it's something that appears
in the schematic and can be pointed at on the board but just doesn't
have software control. From that point of view if the software that's
being interacted with on the remote processor is in ROM/flash which
won't or can't be realistically be updated (which seems to be mostly the
case) then you can reasonably say the same thing.