[RFC 3/3] mailbox: pl320: Introduce common API driver

[Jassi Brar]

On 29 April 2013 22:36, Mark Langsdorf <mark.langsdorf at calxeda.com> wrote:
> On 04/29/2013 11:57 AM, Jassi Brar wrote:
>> On 29 April 2013 22:14, Suman Anna <s-anna at ti.com> wrote:
>>> On 04/27/2013 01:14 PM, jassisinghbrar at gmail.com wrote:
>>>> From: Jassi Brar <jaswinder.singh at linaro.org>
>>>> Signed-off-by: Jassi Brar <jaswinder.singh at linaro.org>
>>>> ---
>>>>  drivers/cpufreq/highbank-cpufreq.c       |   22 +++-
>>>>  drivers/mailbox/Makefile                 |    2 +-
>>>>  drivers/mailbox/{pl320-ipc.c => pl320.c} |  194 ++++++++++++++++--------------
>>>>  include/linux/pl320-ipc.h                |   17 ---
>>>>  4 files changed, 125 insertions(+), 110 deletions(-)
>>>>  rename drivers/mailbox/{pl320-ipc.c => pl320.c} (51%)
>>>>  delete mode 100644 include/linux/pl320-ipc.h
>>>>
>>>> diff --git a/drivers/cpufreq/highbank-cpufreq.c b/drivers/cpufreq/highbank-cpufreq.c
>>>> index 3118b87..5c057e0 100644
>>>> --- a/drivers/cpufreq/highbank-cpufreq.c
>>>> +++ b/drivers/cpufreq/highbank-cpufreq.c
>>>> @@ -19,7 +19,7 @@
>>>>  #include <linux/cpu.h>
>>>>  #include <linux/err.h>
>>>>  #include <linux/of.h>
>>>> -#include <linux/pl320-ipc.h>
>>>> +#include <linux/mailbox_client.h>
>>>>  #include <linux/platform_device.h>
>>>>
>>>>  #define HB_CPUFREQ_CHANGE_NOTE       0x80000001
>>>> @@ -29,8 +29,26 @@
>>>>  static int hb_voltage_change(unsigned int freq)
>>>>  {
>>>>       u32 msg[HB_CPUFREQ_IPC_LEN] = {HB_CPUFREQ_CHANGE_NOTE, freq / 1000000};
>>>> +     struct ipc_client cl;
>>>> +     int ret = -ETIMEDOUT;
>>>> +     void *chan;
>>>>
>>>> -     return pl320_ipc_transmit(msg);
>>>> +     cl.rxcb = NULL;
>>>> +     cl.txcb = NULL;
>>>> +     cl.tx_block = true;
>>>> +     cl.tx_tout = 1000; /* 1 sec */
>>>> +     cl.cntlr_data = NULL;
>>>> +     cl.knows_txdone = false;
>>>> +     cl.chan_name = "pl320:A9_to_M3";
>>>> +
>>>> +     chan = ipc_request_channel(&cl);
>>>> +
>>>> +     if (ipc_send_message(chan, (void *)msg))
>>>> +             ret = msg[1]; /* PL320 updates buffer with FIFO after ACK */
>>>> +
>>>> +     ipc_free_channel(chan);
>>>
>>> I think I understand why you have done this, but do you really want to
>>> request and free every time in the highbank cpufreq driver?
>>>
>> Exactly my aim - make the API light enough to enable the client to
>> use-and-throw. And also because the channel are exclusively assigned,
>> acquire a channel only for as long as you need it.
>
> Do you have any numbers on the performance impact? Our cpufreq
> transition throughput is bad enough without adding additional delays.
>
Sorry no numbers.
However if you look closely you'll find for your usecase the message
directly reaches the controller and the reply from remote is directly
handed to your client driver. And since there is no context for
channel on your platform channel request/free shouldn't fail either
(or you could keep the channel allocated for the lifetime of the
client driver). So I would not expect any longer delays than the
original way.
 Compared with TI's framework of RX-via-notifier you should be better
off with this API, imho.

cheers.