From: jaswinder.singh@linaro.org (Jassi Brar)
To: linux-arm-kernel@lists.infradead.org
Subject: [RFC 2/3] mailbox: Introduce a new common API
Date: Fri, 10 May 2013 15:36:14 +0530 [thread overview]
Message-ID: <CAJe_ZhfFs5svKPABHWdKA83+LhxA5+Q3LKuJyqyRj_4SMMm_hg@mail.gmail.com> (raw)
In-Reply-To: <518C3CE9.1010603@ti.com>
Hello Suman,
On 10 May 2013 05:48, Suman Anna <s-anna@ti.com> wrote:
>> No, please. The controller driver should not implement any policy (of
>> allowing/disallowing requests). It should simply try to do as
>> directed. If the client screwed up even after getting info from
>> platform_data/DT, let it suffer.
>
> This would be the same as a client trying to misconfigure a link, you
> cannot blame on a client screw up.
>
One kernel code should not suspect other kernel code's intentions. The
safety/security is in allowing only well written and trusted code
inside the kernel space.
If a client misconfigures a link, the best we could do is call it
buggy. BTW such a hell-bent client would rather screw up with a BUG(),
what do we do then?
So no please, I am not convinced the controller driver should have
anything to do with allow/disallow policy, or any policy at all.
>>>>>
>>>> Already one call ipc_link_ops.startup() reaches the controller upon
>>>> ipc_request_channel.
>>>>
>>>> Do you mean ?
>>>> ipc_con_ops.startup();
>>>> ipc_link_ops.startup();
>>>
>>> Yes.
>>>
>> And let the controller startup()/shutdown() upon every
>> link_request/release?
>
> Yes, and the controller driver can take care of any ref-counting or
> whatever other logic it needs to maintain. You see this outside in the
> previous mailbox code, but that is not the responsibility of the core code.
>
The pm_runtime imbalance that you mentioned is the result of OMAP's
API trying to do just that.
>>
>> BTW, I have seen my 2 controllers, OMAP, PL320 and read the dbx500
>> driver. Unsurprisingly none of these have any use for what you call a
>> special ipc_con_ops.startup(). Lets say if the call were there, what
>> would the OMAP put in it?
>
> Enabling the clock for the device. The clock is for the entire IP, a
> link has no power/clock dependencies.
>
OMAP controller driver doesn't do clocks.
It does do rpm though, which does a better job of what OMAP needs.
Please see how pm_runtime_xxx calls are made without worry at each
ipc_link_ops.startup/shutdown.
For controllers that do do clocks, having contiguous
ipc_con_ops.startup();
ipc_link_ops.startup();
doesn't save them any complexity. They still have to keep track of
first ipc_link_ops.startup() call in ipc_con_ops.startup() and last
ipc_link_ops.shutdown() call in ipc_con_ops.shutdown()
Which they could very well do in link_ops, if not rpm.
>>
>>>>> Yeah, the pm_runtime_enable cannot be called twice when mailbox is
>>>>> invoked from multiple clients on separate links, so there has to a
>>>>> controller-level logic/ref-counting for that. The clocking for us is on
>>>>> the controller.
>>>>>
>>>> No. You could call pm_runtime_enable/disable any number of times as
>>>> long as they are balanced. The core does refcounting.
>>>
>>> Exactly, as long as they are balanced. I have two clients dealing with
>>> two remotes (using two links) so pm_runtime_enable on the h/w block
>>> needs to be called only when the first one comes in.
>>>
>> Actually you just gave another reason why the API messing around with
>> controller's power state is a bad idea.
>
> Where do you expect to power up the device (obviously this depends on
> the SoC, and its functional purpose)?
>
Use RPM. Just like OMAP controller driver already does -
unconditionally call pm_runtime_get upon each link request and
pm_runtime_put upon each link release. Neither the controller nor the
API would have to do refcounting. If rpm isn't supported, only then
the controller should do refcounting within linp_ops.startup/shutdown
(anyways you want the con_ops and link_ops to be always contiguous).
>> See how mailbox_startup() tries to balance mbox->ops->startup() and
>> mailbox_fini() the mbox->ops->shutdown() That's very fragile and the
>> cause of imbalance between rpm enable/disable, unless your clients are
>> buggy.
>
> Yeah, it is kinda messed up in the existing code, the startup defined
> there is really for the controller and not the link, and that's why you
> see all the ref-counting balancing logic. The rpm enable/disable being
> called is on the controller's dev, not the link's dev - maybe that's
> what confused you.
>
I wrote the driver for this api, so I do realize the rpm is on
controller. Links are not devices anyways.
cheers,
jassi
next prev parent reply other threads:[~2013-05-10 10:06 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-03-13 3:23 [PATCHv3 00/14] drivers: mailbox: framework creation Suman Anna
2013-03-21 11:39 ` Linus Walleij
2013-03-21 23:22 ` Stephen Rothwell
2013-03-21 23:37 ` Anna, Suman
2013-04-21 2:40 ` Jassi Brar
2013-04-22 15:56 ` Anna, Suman
2013-04-23 4:51 ` Jassi Brar
2013-04-23 19:20 ` Anna, Suman
2013-04-23 23:30 ` Andy Green
2013-04-24 4:39 ` Jassi Brar
2013-04-24 8:08 ` Loic PALLARDY
2013-04-24 8:56 ` Jassi Brar
2013-04-24 23:16 ` Suman Anna
2013-04-25 5:20 ` Jassi Brar
2013-04-25 22:29 ` Suman Anna
2013-04-25 23:51 ` Andy Green
2013-04-26 3:46 ` Jassi Brar
2013-04-27 1:04 ` Suman Anna
2013-04-27 1:48 ` Andy Green
2013-04-29 15:32 ` Suman Anna
2013-04-27 4:51 ` Jassi Brar
2013-04-27 18:05 ` [PATCH 1/3] mailbox: rename pl320-ipc specific mailbox.h jaswinder.singh at linaro.org
2013-04-29 12:46 ` Mark Langsdorf
2013-04-29 16:00 ` [PATCHv3 00/14] drivers: mailbox: framework creation Suman Anna
2013-04-29 16:49 ` Jassi Brar
[not found] ` <1367086496-28647-1-git-send-email-jaswinder.singh@linaro.org>
2013-04-29 16:44 ` [RFC 3/3] mailbox: pl320: Introduce common API driver Suman Anna
2013-04-29 16:57 ` Jassi Brar
2013-04-29 17:06 ` Mark Langsdorf
2013-04-29 17:28 ` Jassi Brar
[not found] ` <1367086474-28614-1-git-send-email-jaswinder.singh@linaro.org>
2013-05-04 2:20 ` [RFC 2/3] mailbox: Introduce a new common API Suman Anna
2013-05-04 19:08 ` Jassi Brar
2013-05-06 23:45 ` Suman Anna
2013-05-07 7:40 ` Jassi Brar
2013-05-07 21:48 ` Suman Anna
2013-05-08 5:44 ` Jassi Brar
2013-05-09 1:25 ` Suman Anna
2013-05-09 16:35 ` Jassi Brar
2013-05-10 0:18 ` Suman Anna
2013-05-10 10:06 ` Jassi Brar [this message]
2013-05-10 16:41 ` Suman Anna
2013-04-24 7:39 ` [PATCHv3 00/14] drivers: mailbox: framework creation Loic PALLARDY
2013-04-24 7:59 ` Jassi Brar
2013-04-24 8:39 ` Loic PALLARDY
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJe_ZhfFs5svKPABHWdKA83+LhxA5+Q3LKuJyqyRj_4SMMm_hg@mail.gmail.com \
--to=jaswinder.singh@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).