[PATCHv3 00/14] drivers: mailbox: framework creation

[PATCHv3 00/14] drivers: mailbox: framework creation

[Suman Anna]

Hi,
Please find the updated mailbox patch series for pulling into linux-next.
The series is rebased on top of 3.9-rc2, and includes one new patch to
rename an existing mailbox.h added as part of the highbank cpufreq
support for 3.9 merge window [1]. 

The rest of the patches are mostly unchanged from the previous version,
other than the required changes as part of rebasing. The main changes
are:
- a new patch to rename existing mailbox.h to pl320-ipc.h (patch 1)
- updated patch to fix cleanup issues in probe & remove of omap2 mailbox
  file including minor variable name changes in hwmod files (patch 2)
- includes the updated dbx500 mailbox patch addressing review
  comments, same as [2] (patch 11)
- removes the MULTIPLATFORM Kconfig dependencies added to mailbox and
  remoteproc drivers for 3.9
- minor rebase changes include whitespace formatting

I am wondering if Patch 1 can be absorbed into 3.9 itself, since the
PL320 IPC and associated header file is introduced in 3.9-rc1.

Stephen,
I have hosted the series at [3]. Can you pull this into linux-next
sometime next week?

v2: [4]
After commit e8d3d47 (ARM: OMAP2+: Drop plat/cpu.h for omap2plus), the
cpu_is_xxx() checks for OMAP are restricted to arch/arm/mach-omap2. The
series includes 4 new patches, first patch removes these arch specific
calls of OMAP mailbox driver code (dependencies with soc.h), and the
last three patches include minor fixes in mailbox driver code.

This series is based on v3.8-rc7 and includes the necessary updates/fixes
required for validating remoteproc on OMAP4 and tidspbridge on OMAP3.

Other changes include:
- adaptations to remoteproc and tidspbridge to use the new mailbox
  api, and relying on the pdata field in the mailbox_msg structure
  instead of the previous header field (addressing review comments)
- ST-Ericsson driver update
- Kconfig fixes to fix build errors and choose proper ARCH dependencies
- 3 new patches for minor fixes in mailbox driver code
- rebased to include the devinit/devexit cleanup changes
- checkpatch errors/warnings fixes

v1:
OMAP and ST-Ericsson platforms are both using mailbox to communicate with some coprocessors.
Based on OMAP existing mailbox framework, this series proposes a generic framework, living under drivers/mailbox.

This series:
- moves omap-mailbox framework to a newly drivers/mailbox folder
  (part of plat-omap code cleaning)
- creates API header file
- replaces "omap" prefix by "mailbox"
- opens interface and make framework independent from omap HW
- adapts existing omap1 and omap2 drivers to new changes
- creates dbx500 mailbox driver for ST-Ericsson platforms

[1] http://www.spinics.net/lists/cpufreq/msg04031.html
[2] http://marc.info/?l=linux-omap&m=136079313704751&w=2
[3] https://github.com/sumananna/mailbox/commits/dbx500-prcmu-mailbox
[4] http://marc.info/?l=linux-omap&m=136064540007076&w=2

Loic Pallardy (7):
  mailbox: rename omap_mbox in mailbox
  mailbox: create opened message type
  mailbox: change protection mechanisms
  mailbox: add shared memory mailbox type
  mailbox: add IRQF_NO_SUSPEND flag
  mailbox: add no_irq send message
  mailbox: create dbx500 mailbox driver

Omar Ramirez Luna (2):
  mailbox: OMAP: introduce mailbox framework
  mailbox: split internal header from API header

Suman Anna (5):
  mailbox: rename pl320-ipc specific mailbox.h
  ARM: OMAP2+: mbox: remove dependencies with soc.h
  mailbox/omap: check iomem resource before dereferencing it
  mailbox: check for NULL nb in mailbox_put
  mailbox: call request_irq after mbox queues are allocated

 .../devicetree/bindings/mailbox/dbx500-mailbox.txt |  27 +
 arch/arm/configs/omap1_defconfig                   |   3 +-
 arch/arm/mach-omap1/Makefile                       |   4 -
 arch/arm/mach-omap1/mailbox.c                      | 199 -------
 arch/arm/mach-omap2/Makefile                       |   3 -
 arch/arm/mach-omap2/devices.c                      |  13 +-
 arch/arm/mach-omap2/mailbox.c                      | 430 --------------
 arch/arm/mach-omap2/omap_hwmod_2420_data.c         |  12 +
 arch/arm/mach-omap2/omap_hwmod_2430_data.c         |  11 +
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c         |  11 +
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c         |  13 +
 arch/arm/plat-omap/Kconfig                         |  16 -
 arch/arm/plat-omap/Makefile                        |   3 -
 arch/arm/plat-omap/include/plat/mailbox.h          | 105 ----
 arch/arm/plat-omap/mailbox.c                       | 435 --------------
 drivers/cpufreq/highbank-cpufreq.c                 |   2 +-
 drivers/mailbox/Kconfig                            |  41 ++
 drivers/mailbox/Makefile                           |   5 +
 drivers/mailbox/mailbox-dbx500.c                   | 648 +++++++++++++++++++++
 drivers/mailbox/mailbox-omap1.c                    | 229 ++++++++
 drivers/mailbox/mailbox-omap2.c                    | 370 ++++++++++++
 drivers/mailbox/mailbox.c                          | 552 ++++++++++++++++++
 drivers/mailbox/mailbox_internal.h                 |  70 +++
 drivers/mailbox/pl320-ipc.c                        |   2 +-
 drivers/remoteproc/Kconfig                         |   3 +-
 drivers/remoteproc/omap_remoteproc.c               |  36 +-
 drivers/staging/tidspbridge/Kconfig                |   3 +-
 drivers/staging/tidspbridge/core/_tiomap.h         |   2 +-
 drivers/staging/tidspbridge/core/chnl_sm.c         |   8 +-
 drivers/staging/tidspbridge/core/io_sm.c           |   5 +-
 drivers/staging/tidspbridge/core/tiomap3430.c      |   6 +-
 drivers/staging/tidspbridge/core/tiomap3430_pwr.c  |   6 +-
 drivers/staging/tidspbridge/core/tiomap_io.c       |   9 +-
 .../tidspbridge/include/dspbridge/host_os.h        |   2 +-
 include/linux/mailbox.h                            |  52 +-
 include/linux/pl320-ipc.h                          |  17 +
 include/linux/platform_data/mailbox-dbx500.h       |  12 +
 include/linux/platform_data/mailbox-omap.h         |  53 ++
 38 files changed, 2170 insertions(+), 1248 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mailbox/dbx500-mailbox.txt
 delete mode 100644 arch/arm/mach-omap1/mailbox.c
 delete mode 100644 arch/arm/mach-omap2/mailbox.c
 delete mode 100644 arch/arm/plat-omap/include/plat/mailbox.h
 delete mode 100644 arch/arm/plat-omap/mailbox.c
 create mode 100644 drivers/mailbox/mailbox-dbx500.c
 create mode 100644 drivers/mailbox/mailbox-omap1.c
 create mode 100644 drivers/mailbox/mailbox-omap2.c
 create mode 100644 drivers/mailbox/mailbox.c
 create mode 100644 drivers/mailbox/mailbox_internal.h
 create mode 100644 include/linux/pl320-ipc.h
 create mode 100644 include/linux/platform_data/mailbox-dbx500.h
 create mode 100644 include/linux/platform_data/mailbox-omap.h

-- 
1.8.1.2

[PATCHv3 00/14] drivers: mailbox: framework creation

[Linus Walleij]

On Wed, Mar 13, 2013 at 4:23 AM, Suman Anna <s-anna@ti.com> wrote:

> Please find the updated mailbox patch series for pulling into linux-next.
> The series is rebased on top of 3.9-rc2, and includes one new patch to
> rename an existing mailbox.h added as part of the highbank cpufreq
> support for 3.9 merge window [1].

ARM SoC folks:

would you consider pulling this stuff into the ARM SoC tree?

It turns out that ux500 multiplatform support is sort of relying
on this refactoring since it helps us to break apart the huge
PRCMU driver.

I am proceeding with my multiplatform work but things like
this not being upstream will make the patches look ugly
and I cannot quite consider it properly done before this is
fixed too.

Yours,
Linus Walleij

[PATCHv3 00/14] drivers: mailbox: framework creation

[Stephen Rothwell]

Hi Suman,

On Tue, 12 Mar 2013 22:23:41 -0500 Suman Anna <s-anna@ti.com> wrote:
>
> Stephen,
> I have hosted the series at [3]. Can you pull this into linux-next
> sometime next week?

> [3] https://github.com/sumananna/mailbox/commits/dbx500-prcmu-mailbox

Please quote git URLs ... I guessed you meant
git://github.com/sumananna/mailbox.git, branch dbx500-prcmu-mailbox ?

Added from today.

Thanks for adding your subsystem tree as a participant of linux-next.  As
you may know, this is not a judgment of your code.  The purpose of
linux-next is for integration testing and to lower the impact of
conflicts between subsystems in the next merge window. 

You will need to ensure that the patches/commits in your tree/series have
been:
     * submitted under GPL v2 (or later) and include the Contributor's
	Signed-off-by,
     * posted to the relevant mailing list,
     * reviewed by you (or another maintainer of your subsystem tree),
     * successfully unit tested, and 
     * destined for the current or next Linux merge window.

Basically, this should be just what you would send to Linus (or ask him
to fetch).  It is allowed to be rebased if you deem it necessary.

-- 
Cheers,
Stephen Rothwell 
sfr at canb.auug.org.au

Legal Stuff:
By participating in linux-next, your subsystem tree contributions are
public and will be included in the linux-next trees.  You may be sent
e-mail messages indicating errors or other issues when the
patches/commits from your subsystem tree are merged and tested in
linux-next.  These messages may also be cross-posted to the linux-next
mailing list, the linux-kernel mailing list, etc.  The linux-next tree
project and IBM (my employer) make no warranties regarding the linux-next
project, the testing procedures, the results, the e-mails, etc.  If you
don't agree to these ground rules, let me know and I'll remove your tree
from participation in linux-next.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130322/1e76c8d4/attachment-0001.sig>

[PATCHv3 00/14] drivers: mailbox: framework creation

[Anna, Suman]

> >
> > Stephen,
> > I have hosted the series at [3]. Can you pull this into linux-next
> > sometime next week?
> 
> > [3] https://github.com/sumananna/mailbox/commits/dbx500-prcmu-mailbox
> 
> Please quote git URLs ... I guessed you meant
> git://github.com/sumananna/mailbox.git, branch dbx500-prcmu-mailbox ?
> 
> Added from today.

Yes, that's correct. Thanks Stephen.

Regards
Suman

[PATCHv3 00/14] drivers: mailbox: framework creation

[Jassi Brar]

Hi Suman,

  [Resending with mailing-list CCed this time. For some reason LKML
doesn't appear in reply-to despite this message being fwd'ed from
there]

On Wed, Mar 13, 2013 at 8:53 AM, Suman Anna <s-anna@ti.com> wrote:
> Hi,
> Please find the updated mailbox patch series for pulling into linux-next.
> The series is rebased on top of 3.9-rc2, and includes one new patch to
> rename an existing mailbox.h added as part of the highbank cpufreq
> support for 3.9 merge window [1].
>
I am to implement IPC protocol and client drivers on a platform that
has a master processor having control over critical h/w resources,
like clock and power management, to be used by the MPCore running
linux. And there are 2 such IPC controllers - one for communication
within the MPCore and another for between MPCore and the 'Master'.

I have a few concerns about this api as follows...

a) No documentation. Usually the header would have proper
documentation of data structures and info for users of both side of
the api.

b) The api is not scalable. The assumption of just one IPC controller
in the platform is hardcoded.

c) There seems to be no consistent design - mailbox_ops has 12
callback hooks. Some, save_ctx, restore_ctx, enable_irq, disable_irq
are exported for no apparent reason (legacy of a legacy - OMAP), while
other hooks are kept private to the api.
I believe OMAP had valid reasons to make IPC clients save/restore
context of the IPC controller, but I don't think that is the case in
general. I2C client drivers don't save/restore context of I2C
controller's, why should IPC's?  Similarly enable/disable_irq of the
controller seem too intrusive for a client driver.

mailbox_ops.mailbox_type_t makes no sense to me. At least not without
documentation, though I doubt any amount of documentation could help
it - mailbox.c doesn't even read the member. Btw, isn't every mailbox
a MBOX_SHARED_MEM_TYPE? A part of data passed via FIFO/REG could point
to location in shared memory that might have full command/payload for
the message. Or did I get the meaning of MBOX_SHARED_MEM_TYPE wrong in
the absence of documentation?

The api seems to worry too much about the low-level details of the IPC
controller (save/restore context, enable/disable_irq and ack_irq),
while it fails to provide a tight well-defined interface to client
drivers.

There are also some code issues, which might come as replies to
respective patches.

Regards,
-Jassi

[PATCHv3 00/14] drivers: mailbox: framework creation

[Anna, Suman]

> 
> Hi Suman,
> 
>   [Resending with mailing-list CCed this time. For some reason LKML doesn't
> appear in reply-to despite this message being fwd'ed from there]
> 
> On Wed, Mar 13, 2013 at 8:53 AM, Suman Anna <s-anna@ti.com> wrote:
> > Hi,
> > Please find the updated mailbox patch series for pulling into linux-next.
> > The series is rebased on top of 3.9-rc2, and includes one new patch to
> > rename an existing mailbox.h added as part of the highbank cpufreq
> > support for 3.9 merge window [1].
> >
> I am to implement IPC protocol and client drivers on a platform that has a master
> processor having control over critical h/w resources, like clock and power
> management, to be used by the MPCore running linux. And there are 2 such IPC
> controllers - one for communication within the MPCore and another for between
> MPCore and the 'Master'.
> 
> I have a few concerns about this api as follows...

Thank Jassi for your comments. I am aware of some of the concerns, Andy Green had highlighted (a) & (b) as well previously, and I am actually working through the next set of patches to fix these. I will post them sometime next week. 

> 
> a) No documentation. Usually the header would have proper documentation of
> data structures and info for users of both side of the api.

I will fix the documentation portion for 3.10. I will also add a TODO as part of the documentation so that it highlights the gaps and work-items.

> 
> b) The api is not scalable. The assumption of just one IPC controller in the
> platform is hardcoded.

Yes, this is a major concern and I will be handling this in the upcoming patches . The current code was primarily based on moving the OMAP mailbox code and expanding it to support the ST DBX500 mailbox. As it is today, it doesn't scale to support multiple IP instances/controllers.

> 
> c) There seems to be no consistent design - mailbox_ops has 12 callback hooks.
> Some, save_ctx, restore_ctx, enable_irq, disable_irq are exported for no
> apparent reason (legacy of a legacy - OMAP), while other hooks are kept private
> to the api.
> I believe OMAP had valid reasons to make IPC clients save/restore context of the
> IPC controller, but I don't think that is the case in general. I2C client drivers don't
> save/restore context of I2C controller's, why should IPC's?  Similarly
> enable/disable_irq of the controller seem too intrusive for a client driver.

Yep, these are exported because of the OMAP legacy TIDSPBRIDGE stack, and should vanish with the addition of runtime_pm support.

> 
> mailbox_ops.mailbox_type_t makes no sense to me. At least not without
> documentation, though I doubt any amount of documentation could help it -
> mailbox.c doesn't even read the member. Btw, isn't every mailbox a
> MBOX_SHARED_MEM_TYPE? A part of data passed via FIFO/REG could point to
> location in shared memory that might have full command/payload for the
> message. Or did I get the meaning of MBOX_SHARED_MEM_TYPE wrong in the
> absence of documentation?

Yes, this needs to be removed, it is a carry-over from the OMAP mailbox code.

> 
> The api seems to worry too much about the low-level details of the IPC controller
> (save/restore context, enable/disable_irq and ack_irq), while it fails to provide a
> tight well-defined interface to client drivers.
> 
> There are also some code issues, which might come as replies to respective
> patches.

Thanks for the review of the patches. I will await your comments, and will address them as incremental patches. 

Regards
Suman

[PATCHv3 00/14] drivers: mailbox: framework creation

[Jassi Brar]

Hi Suman,

On Mon, Apr 22, 2013 at 9:26 PM, Anna, Suman <s-anna@ti.com> wrote:
>>
>> a) No documentation. Usually the header would have proper documentation of
>> data structures and info for users of both side of the api.
>
> I will fix the documentation portion for 3.10. I will also add a TODO as part of the documentation so that it highlights the gaps and work-items.
>
>>
>> b) The api is not scalable. The assumption of just one IPC controller in the
>> platform is hardcoded.
>
> Yes, this is a major concern and I will be handling this in the upcoming patches . The current code was primarily based on moving the OMAP mailbox code and expanding it to support the ST DBX500 mailbox. As it is today, it doesn't scale to support multiple IP instances/controllers.
>
>>
>> c) There seems to be no consistent design - mailbox_ops has 12 callback hooks.
>> Some, save_ctx, restore_ctx, enable_irq, disable_irq are exported for no
>> apparent reason (legacy of a legacy - OMAP), while other hooks are kept private
>> to the api.
>> I believe OMAP had valid reasons to make IPC clients save/restore context of the
>> IPC controller, but I don't think that is the case in general. I2C client drivers don't
>> save/restore context of I2C controller's, why should IPC's?  Similarly
>> enable/disable_irq of the controller seem too intrusive for a client driver.
>
> Yep, these are exported because of the OMAP legacy TIDSPBRIDGE stack, and should vanish with the addition of runtime_pm support.
>
>>
>> mailbox_ops.mailbox_type_t makes no sense to me. At least not without
>> documentation, though I doubt any amount of documentation could help it -
>> mailbox.c doesn't even read the member. Btw, isn't every mailbox a
>> MBOX_SHARED_MEM_TYPE? A part of data passed via FIFO/REG could point to
>> location in shared memory that might have full command/payload for the
>> message. Or did I get the meaning of MBOX_SHARED_MEM_TYPE wrong in the
>> absence of documentation?
>
> Yes, this needs to be removed, it is a carry-over from the OMAP mailbox code.
>
>>
>> The api seems to worry too much about the low-level details of the IPC controller
>> (save/restore context, enable/disable_irq and ack_irq), while it fails to provide a
>> tight well-defined interface to client drivers.
>>
>> There are also some code issues, which might come as replies to respective
>> patches.
>
> Thanks for the review of the patches. I will await your comments, and will address them as incremental patches.
>
So we agree
a) Documentation is missing.
b) mailbox_type_t, save_ctx, restore_ctx, ack_irq, enable_irq,
disable_irq need to be removed.
c) Support for more than one IPC controllers is needed.

Out of 11 exported functions, we'll be left with the 5 trivial ones
mailbox_un/register, mailbox_get/put and mailbox_msg_send.
mailbox_msg_send_no_irq and mailbox_msg_send_receive_no_irq are STE(?)
specific quirky requirements, which should be possible to simulate
over the mailbox API if designed well.

Documentation wise, we'd need documentation for what we finally wanna
have, not the current implementation.

There are also some desired features in a good API:-

a) The API should be simple and efficient, i.e, submission of requests
by clients and notifications of data received by the API should be
possible from atomic context - most of the IPC is going to be about
exchanging 'commands'. The API shouldn't add to the latencies.

b) It would be very useful to allow submission of a new request from
the completion callback of last one.

c) The request/free/ack_irq on behalf of the IPC controller driver
should be no business of the API. The API design should only assume a
simple but generic enough h/w model beneath it and provide support to
h/w that doesn't have an expected feature.
 For example, API should assume the IPC controller can detect and
report when the remote asserts Ready-To-Receive (RTR). This is when
the API callbacks .tx_done(mssg) on the client. If some h/w doesn't
have RTR assertion interrupt, the API should provide optional feature
to poll the controller periodically.

(d) The 'struct mailbox_msg' should be just an opaque void* - the
client/protocol driver typecasts to void* the IPC controller specific
message structure. API shouldn't aim the impossible of providing a
generic message format.

(a) and (b) are already proved to be useful and supported by a similar
API - DMAEngine.

As you see there would be not much of the present left eventually. So
I wonder if should sculpt a new api out of TI's or start from scratch?
One of my controllers is similar to 'pl320' (which you left out
converting to the API). I am in process of implementing all this
assuming it is OK to keep a controller out of this API :)  So maybe we
can collaborate on a new implementation from scratch.

Regards,
Jassi

[PATCHv3 00/14] drivers: mailbox: framework creation

[Anna, Suman]

Hi Jassi,

> 
> On Mon, Apr 22, 2013 at 9:26 PM, Anna, Suman <s-anna@ti.com> wrote:
> >>
> >> a) No documentation. Usually the header would have proper
> >> documentation of data structures and info for users of both side of the api.
> >
> > I will fix the documentation portion for 3.10. I will also add a TODO as part of the
> documentation so that it highlights the gaps and work-items.
> >
> >>
> >> b) The api is not scalable. The assumption of just one IPC controller
> >> in the platform is hardcoded.
> >
> > Yes, this is a major concern and I will be handling this in the upcoming patches .
> The current code was primarily based on moving the OMAP mailbox code and
> expanding it to support the ST DBX500 mailbox. As it is today, it doesn't scale to
> support multiple IP instances/controllers.
> >
> >>
> >> c) There seems to be no consistent design - mailbox_ops has 12 callback
> hooks.
> >> Some, save_ctx, restore_ctx, enable_irq, disable_irq are exported for
> >> no apparent reason (legacy of a legacy - OMAP), while other hooks are
> >> kept private to the api.
> >> I believe OMAP had valid reasons to make IPC clients save/restore
> >> context of the IPC controller, but I don't think that is the case in
> >> general. I2C client drivers don't save/restore context of I2C
> >> controller's, why should IPC's?  Similarly enable/disable_irq of the controller
> seem too intrusive for a client driver.
> >
> > Yep, these are exported because of the OMAP legacy TIDSPBRIDGE stack, and
> should vanish with the addition of runtime_pm support.
> >
> >>
> >> mailbox_ops.mailbox_type_t makes no sense to me. At least not without
> >> documentation, though I doubt any amount of documentation could help
> >> it - mailbox.c doesn't even read the member. Btw, isn't every mailbox
> >> a MBOX_SHARED_MEM_TYPE? A part of data passed via FIFO/REG could
> >> point to location in shared memory that might have full
> >> command/payload for the message. Or did I get the meaning of
> >> MBOX_SHARED_MEM_TYPE wrong in the absence of documentation?
> >
> > Yes, this needs to be removed, it is a carry-over from the OMAP mailbox code.
> >
> >>
> >> The api seems to worry too much about the low-level details of the
> >> IPC controller (save/restore context, enable/disable_irq and
> >> ack_irq), while it fails to provide a tight well-defined interface to client drivers.
> >>
> >> There are also some code issues, which might come as replies to
> >> respective patches.
> >
> > Thanks for the review of the patches. I will await your comments, and will
> address them as incremental patches.
> >
> So we agree
> a) Documentation is missing.
> b) mailbox_type_t, save_ctx, restore_ctx, ack_irq, enable_irq, disable_irq need
> to be removed.

The mailbox_type_t can be removed right away, but for others, we should give some time for current users to migrate before we remove it completely. 

> c) Support for more than one IPC controllers is needed.
> 
> Out of 11 exported functions, we'll be left with the 5 trivial ones
> mailbox_un/register, mailbox_get/put and mailbox_msg_send.
> mailbox_msg_send_no_irq and mailbox_msg_send_receive_no_irq are STE(?)
> specific quirky requirements, which should be possible to simulate over the
> mailbox API if designed well.

Loic can comment on the existing xxx_no_irq API. The pl320_transmit is somewhat similar to mailbox_msg_send_receive_no_irq, without the irq part.

> 
> Documentation wise, we'd need documentation for what we finally wanna have,
> not the current implementation.
> 
> There are also some desired features in a good API:-
> 
> a) The API should be simple and efficient, i.e, submission of requests by clients
> and notifications of data received by the API should be possible from atomic
> context - most of the IPC is going to be about exchanging 'commands'. The API
> shouldn't add to the latencies.

I think we will have need for both types. It really depends on the purpose of the IPC h/w - some would be simple messaging and may not need the atomic constraints. Sure enough, atomic context is the stricter one, so we can add that. I don't see any users of it currently though.

> 
> b) It would be very useful to allow submission of a new request from the
> completion callback of last one.
> 
> c) The request/free/ack_irq on behalf of the IPC controller driver should be no
> business of the API. The API design should only assume a simple but generic
> enough h/w model beneath it and provide support to h/w that doesn't have an
> expected feature.
>  For example, API should assume the IPC controller can detect and report when
> the remote asserts Ready-To-Receive (RTR). This is when the API callbacks
> .tx_done(mssg) on the client. If some h/w doesn't have RTR assertion interrupt,
> the API should provide optional feature to poll the controller periodically.

Some of these are already part of the mailbox ops. FWIW, I don?t see a need for a .tx_done callback, as this state can really be maintained by the driver implementation itself. The mailbox_msg_send can return an error appropriately based on whether the h/w is ready or not. The current code has the support for queuing up a certain number of messages in case the h/w is busy, but I am planning to change this to a device-based property so that they can choose if it needs that support or not.

> 
> (d) The 'struct mailbox_msg' should be just an opaque void* - the client/protocol
> driver typecasts to void* the IPC controller specific message structure. API
> shouldn't aim the impossible of providing a generic message format.

The size parameter would still be needed. Depending on the h/w, it can be just an u32 or a series of bytes, and even in the latter case, it is not guaranteed that all messages transmitted will occupy the entire h/w shared memory data packet. I can see the current header field getting absorbed into the opaque void * structure for the ST mailbox driver. The size and ptr together provide a very generic message format.

> 
> (a) and (b) are already proved to be useful and supported by a similar API -
> DMAEngine.
> 
> As you see there would be not much of the present left eventually. So I wonder
> if should sculpt a new api out of TI's or start from scratch?
> One of my controllers is similar to 'pl320' (which you left out converting to the
> API). I am in process of implementing all this assuming it is OK to keep a controller
> out of this API :)  So maybe we can collaborate on a new implementation from
> scratch.

This series missed the 3.9 merge window, and is currently slated for getting merged into 3.10. The PL320 made it into 3.9 itself (I wasn't aware of it until I saw it in mainline) and created the drivers/mailbox folder.  I would think it would be relatively straight-forward to adopt it to the mailbox API, as it has only 3 API. We should be doing incremental changes on top of this series, as most of the base API would still be the same. The current series is helping out with couple of efforts, the breaking up of the PRCMU code and on the multiplatform support for OMAP with mailbox enabled. We can definitely collaborate on the improvements. Andy Green would also be interested, as he is also looking into adopting the mailbox API.

Regards
Suman

[PATCHv3 00/14] drivers: mailbox: framework creation

[Andy Green]

On 24/04/13 03:20, the mail apparently from Anna, Suman included:

Hi Suman -

> This series missed the 3.9 merge window, and is currently slated for
> getting merged into 3.10. The PL320 made it into 3.9 itself (I wasn't
> aware of it until I saw it in mainline) and created the
> drivers/mailbox folder.  I would think it would be relatively
> straight-forward to adopt it to the mailbox API, as it has only 3
> API. We should be doing incremental changes on top of this series, as
> most of the base API would still be the same. The current series is
> helping out with couple of efforts, the breaking up of the PRCMU code
> and on the multiplatform support for OMAP with mailbox enabled. We
> can definitely collaborate on the improvements. Andy Green would also
> be interested, as he is also looking into adopting the mailbox API.

To clarify Jassi works on my team, after I wrote two Mailbox drivers for 
two different IP on the chips we're working on, I handed them off to him 
and Jassi's working on further integration using those drivers.  So 
we're both doing the same thing.

 From my POV I am very happy you made the new API - before that there 
was only PL320 sitting there and nothing you could call an API.  Once I 
understood the approach (no docs was a bit painful) I was able to 
implement both drivers we needed with what you have.

The main problem I have with it we discussed in direct mail previously, 
since we have two different mailbox IP, we need to improve the register 
/ unregister so it can cope, right now it's unnecessarily limited to one 
mailbox driver.

That "there can only be one" approach also leaked out into the drivers 
having filescope statics for things that should have been instantiated 
per-mailbox device (including device naming as literals, rather than 
mbox%d needed if there can be multiple drivers).  In my driver 
implementations I moved them to live in the per-device priv struct and 
stored the names in there too.

The other point I mentioned before was the FIFO, it's always there and 
size set by CONFIG_ stuff.  Actually it would be better if it was set 
per mailbox or per mailbox driver at runtime.  For one of the IPs, we 
will have another driver mediating access to the mailbox that enforces a 
single client access covering possibly multiple mailbox messages.  Under 
those conditions, a fifo isn't really meaningful.  But that's less of a 
problem.

As I say I was very happy to see you addressing the lack of an API, 
Jassi though is working deeper with it than just making the mailbox 
drivers as I did; he's using the API from other consumer drivers so he 
may have a different set of concerns or, looking at what he's written 
here, opportunities.

-Andy

-- 
Andy Green | Fujitsu Landing Team Leader
Linaro.org ? Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  -
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog

[PATCHv3 00/14] drivers: mailbox: framework creation

[Jassi Brar]

Hi Suman,

[please limit replies to not more than 80 columns per line]

On 24 April 2013 00:50, Anna, Suman <s-anna@ti.com> wrote:

>>
>> Documentation wise, we'd need documentation for what we finally wanna have,
>> not the current implementation.
>>
>> There are also some desired features in a good API:-
>>
>> a) The API should be simple and efficient, i.e, submission of requests by clients
>> and notifications of data received by the API should be possible from atomic
>> context - most of the IPC is going to be about exchanging 'commands'. The API
>> shouldn't add to the latencies.
>
> I think we will have need for both types. It really depends on the purpose of the IPC h/w - some would be simple messaging and may not need the atomic constraints. Sure enough, atomic context is the stricter one, so we can add that. I don't see any users of it currently though.
>
IMHO the API authors should take the other POV :  if _majority_ of
clients _require_ non-atomic context, only then the API should support
non-atomic version of the calls too. Otherwise let the minority of
drivers schedule their sleepable tasks from the API's atomic
callbacks.
 The non-atomic API falls flat should just a single client comes with
very low latency requirements.

>>  For example, API should assume the IPC controller can detect and report when
>> the remote asserts Ready-To-Receive (RTR). This is when the API callbacks
>> .tx_done(mssg) on the client. If some h/w doesn't have RTR assertion interrupt,
>> the API should provide optional feature to poll the controller periodically.
>
> Some of these are already part of the mailbox ops. FWIW, I don?t see a need for a .tx_done callback, as this state can really be maintained by the driver implementation itself. The mailbox_msg_send can return an error appropriately based on whether the h/w is ready or not. The current code has the support for queuing up a certain number of messages in case the h/w is busy, but I am planning to change this to a device-based property so that they can choose if it needs that support or not.
>
Not really. The TI's api does buffer requests from a client but it
does not tell the client when the message is actually sent to the
remote over the h/w channel.

 Consider this example for better understanding, I must send a
particular packet to the remote before I could, say, shutdown
properly. The client calls mailbox_msg_send which queues the packet
because the h/w was busy. Since the api does not inform the client
about packet transmission, the client does not know when to shutdown
(the packet may have been sent instantly or it may lie on the queue
for a long time).
 How do you fix this without tx_done callback method?


>>
>> (d) The 'struct mailbox_msg' should be just an opaque void* - the client/protocol
>> driver typecasts to void* the IPC controller specific message structure. API
>> shouldn't aim the impossible of providing a generic message format.
>
> The size parameter would still be needed. Depending on the h/w, it can be just an u32 or a series of bytes, and even in the latter case, it is not guaranteed that all messages transmitted will occupy the entire h/w shared memory data packet. I can see the current header field getting absorbed into the opaque void * structure for the ST mailbox driver. The size and ptr together provide a very generic message format.
>
I am not sure about it. The API has no purpose for the .size (it's
between the controller and the client driver).
Controllers that could send variable length packets could define the
packets format for clients as
 struct foo_packet {
         int size;
         .......
 }
and exchange as typecasted to void* via the API. And this is just one
way of doing it.
The point being, the API should only pass pointer to packet info
between controller and client drivers.

>>
>> As you see there would be not much of the present left eventually. So I wonder
>> if should sculpt a new api out of TI's or start from scratch?
>> One of my controllers is similar to 'pl320' (which you left out converting to the
>> API). I am in process of implementing all this assuming it is OK to keep a controller
>> out of this API :)  So maybe we can collaborate on a new implementation from
>> scratch.
>
> This series missed the 3.9 merge window, and is currently slated for getting merged into 3.10. The PL320 made it into 3.9 itself (I wasn't aware of it until I saw it in mainline) and created the drivers/mailbox folder.  I would think it would be relatively straight-forward to adopt it to the mailbox API, as it has only 3 API. We should be doing incremental changes on top of this series, as most of the base API would still be the same. The current series is helping out with couple of efforts, the breaking up of the PRCMU code and on the multiplatform support for OMAP with mailbox enabled. We can definitely collaborate on the improvements. Andy Green would also be interested, as he is also looking into adopting the mailbox API.
>
I don't think it should into 3.10 just because it missed the 3.9
window, when its quality is doubted.
I am going to have to live with IPC controller code for sometime now,
so I care that I build upon a sound API. And I am more than happy to
help on a proper implementation.

Regards,
Jassi

[PATCHv3 00/14] drivers: mailbox: framework creation

[Loic PALLARDY]

Hi Jassi, Suman,

On 04/23/2013 09:20 PM, Anna, Suman wrote:
> Hi Jassi,
>
>>
>> On Mon, Apr 22, 2013 at 9:26 PM, Anna, Suman<s-anna@ti.com>  wrote:
>>>>
>>>> a) No documentation. Usually the header would have proper
>>>> documentation of data structures and info for users of both side of the api.
>>>
>>> I will fix the documentation portion for 3.10. I will also add a TODO as part of the
>> documentation so that it highlights the gaps and work-items.
>>>
>>>>
>>>> b) The api is not scalable. The assumption of just one IPC controller
>>>> in the platform is hardcoded.
>>>
>>> Yes, this is a major concern and I will be handling this in the upcoming patches .
>> The current code was primarily based on moving the OMAP mailbox code and
>> expanding it to support the ST DBX500 mailbox. As it is today, it doesn't scale to
>> support multiple IP instances/controllers.
>>>
>>>>
>>>> c) There seems to be no consistent design - mailbox_ops has 12 callback
>> hooks.
>>>> Some, save_ctx, restore_ctx, enable_irq, disable_irq are exported for
>>>> no apparent reason (legacy of a legacy - OMAP), while other hooks are
>>>> kept private to the api.
>>>> I believe OMAP had valid reasons to make IPC clients save/restore
>>>> context of the IPC controller, but I don't think that is the case in
>>>> general. I2C client drivers don't save/restore context of I2C
>>>> controller's, why should IPC's?  Similarly enable/disable_irq of the controller
>> seem too intrusive for a client driver.
>>>
>>> Yep, these are exported because of the OMAP legacy TIDSPBRIDGE stack, and
>> should vanish with the addition of runtime_pm support.
>>>
>>>>
>>>> mailbox_ops.mailbox_type_t makes no sense to me. At least not without
>>>> documentation, though I doubt any amount of documentation could help
>>>> it - mailbox.c doesn't even read the member. Btw, isn't every mailbox
>>>> a MBOX_SHARED_MEM_TYPE? A part of data passed via FIFO/REG could
>>>> point to location in shared memory that might have full
>>>> command/payload for the message. Or did I get the meaning of
>>>> MBOX_SHARED_MEM_TYPE wrong in the absence of documentation?
>>>
>>> Yes, this needs to be removed, it is a carry-over from the OMAP mailbox code.
>>>
>>>>
>>>> The api seems to worry too much about the low-level details of the
>>>> IPC controller (save/restore context, enable/disable_irq and
>>>> ack_irq), while it fails to provide a tight well-defined interface to client drivers.
>>>>
>>>> There are also some code issues, which might come as replies to
>>>> respective patches.
>>>
>>> Thanks for the review of the patches. I will await your comments, and will
>> address them as incremental patches.
>>>
>> So we agree
>> a) Documentation is missing.
>> b) mailbox_type_t, save_ctx, restore_ctx, ack_irq, enable_irq, disable_irq need
>> to be removed.
>
> The mailbox_type_t can be removed right away, but for others, we should give some time for current users to migrate before we remove it completely.
>
>> c) Support for more than one IPC controllers is needed.
>>
>> Out of 11 exported functions, we'll be left with the 5 trivial ones
>> mailbox_un/register, mailbox_get/put and mailbox_msg_send.
>> mailbox_msg_send_no_irq and mailbox_msg_send_receive_no_irq are STE(?)
>> specific quirky requirements, which should be possible to simulate over the
>> mailbox API if designed well.
>
> Loic can comment on the existing xxx_no_irq API. The pl320_transmit is somewhat similar to mailbox_msg_send_receive_no_irq, without the irq part.
>
Yes, the xxx_no_irq API has been introduced to answer some STE 
requirements. It must be possible to send some message under atomic 
context for different reasons (latency, during idle/suspend procedures...).
This is not the best way to do, but the goal was to preserve TI legacy 
in a first step. As explained by Suman, this patch series is coming from 
the original TI mailbox framework and is modified step by step to fit 
with all platforms.

>>
>> Documentation wise, we'd need documentation for what we finally wanna have,
>> not the current implementation.
>>
>> There are also some desired features in a good API:-
>>
>> a) The API should be simple and efficient, i.e, submission of requests by clients
>> and notifications of data received by the API should be possible from atomic
>> context - most of the IPC is going to be about exchanging 'commands'. The API
>> shouldn't add to the latencies.
>
> I think we will have need for both types. It really depends on the purpose of the IPC h/w - some would be simple messaging and may not need the atomic constraints. Sure enough, atomic context is the stricter one, so we can add that. I don't see any users of it currently though.
>
>>
>> b) It would be very useful to allow submission of a new request from the
>> completion callback of last one.
>>
>> c) The request/free/ack_irq on behalf of the IPC controller driver should be no
>> business of the API. The API design should only assume a simple but generic
>> enough h/w model beneath it and provide support to h/w that doesn't have an
>> expected feature.
>>   For example, API should assume the IPC controller can detect and report when
>> the remote asserts Ready-To-Receive (RTR). This is when the API callbacks
>> .tx_done(mssg) on the client. If some h/w doesn't have RTR assertion interrupt,
>> the API should provide optional feature to poll the controller periodically.
>
> Some of these are already part of the mailbox ops. FWIW, I don?t see a need for a .tx_done callback, as this state can really be maintained by the driver implementation itself. The mailbox_msg_send can return an error appropriately based on whether the h/w is ready or not. The current code has the support for queuing up a certain number of messages in case the h/w is busy, but I am planning to change this to a device-based property so that they can choose if it needs that support or not.
>
>>
>> (d) The 'struct mailbox_msg' should be just an opaque void* - the client/protocol
>> driver typecasts to void* the IPC controller specific message structure. API
>> shouldn't aim the impossible of providing a generic message format.
>
> The size parameter would still be needed. Depending on the h/w, it can be just an u32 or a series of bytes, and even in the latter case, it is not guaranteed that all messages transmitted will occupy the entire h/w shared memory data packet. I can see the current header field getting absorbed into the opaque void * structure for the ST mailbox driver. The size and ptr together provide a very generic message format.
That's something we discussed with Suman. The mailbox framework should 
be independent from message format. Since mailbox could be base don a 
shared mem or an hw fifo, message size is mandatory to transmit the 
right number of data.

Regards,
Loic
>
>>
>> (a) and (b) are already proved to be useful and supported by a similar API -
>> DMAEngine.
>>
>> As you see there would be not much of the present left eventually. So I wonder
>> if should sculpt a new api out of TI's or start from scratch?
>> One of my controllers is similar to 'pl320' (which you left out converting to the
>> API). I am in process of implementing all this assuming it is OK to keep a controller
>> out of this API :)  So maybe we can collaborate on a new implementation from
>> scratch.
>
> This series missed the 3.9 merge window, and is currently slated for getting merged into 3.10. The PL320 made it into 3.9 itself (I wasn't aware of it until I saw it in mainline) and created the drivers/mailbox folder.  I would think it would be relatively straight-forward to adopt it to the mailbox API, as it has only 3 API. We should be doing incremental changes on top of this series, as most of the base API would still be the same. The current series is helping out with couple of efforts, the breaking up of the PRCMU code and on the multiplatform support for OMAP with mailbox enabled. We can definitely collaborate on the improvements. Andy Green would also be interested, as he is also looking into adopting the mailbox API.
>
> Regards
> Suman

[PATCHv3 00/14] drivers: mailbox: framework creation

[Jassi Brar]

Hi Loic,

On 24 April 2013 13:09, Loic PALLARDY <loic.pallardy@st.com> wrote:
> Hi Jassi, Suman,
>
> Yes, the xxx_no_irq API has been introduced to answer some STE
> requirements. It must be possible to send some message under atomic
> context for different reasons (latency, during idle/suspend procedures...).
> This is not the best way to do, but the goal was to preserve TI legacy
> in a first step. As explained by Suman, this patch series is coming from
> the original TI mailbox framework and is modified step by step to fit
> with all platforms.
>
IMHO a better way is to introduce a clean generically designed API and
convert the existing drivers one at time. Let the TI drivers work as
such for a while until someone converts them to the common API.
Cloning and then complete organ transplantation seems a very
inefficient way to have something new ;)

>>>
>>> (d) The 'struct mailbox_msg' should be just an opaque void* - the client/protocol
>>> driver typecasts to void* the IPC controller specific message structure. API
>>> shouldn't aim the impossible of providing a generic message format.
>>
>> The size parameter would still be needed. Depending on the h/w, it can be just an u32 or a series of bytes, and even in the latter case, it is not guaranteed that all messages transmitted will occupy the entire h/w shared memory data packet. I can see the current header field getting absorbed into the opaque void * structure for the ST mailbox driver. The size and ptr together provide a very generic message format.
> That's something we discussed with Suman. The mailbox framework should
> be independent from message format. Since mailbox could be base don a
> shared mem or an hw fifo, message size is mandatory to transmit the
> right number of data.
>
I too believe the "mailbox framework should be independent from
message format" but _also_ that .size doesn't have to be a part of the
standard format.
Maybe it will help if I know what you guys mean by "shared mem" or an
"hw fifo" mailbox?

Thanks
-Jassi

[PATCHv3 00/14] drivers: mailbox: framework creation

[Loic PALLARDY]

Hi Jassi,

On 04/24/2013 06:39 AM, Jassi Brar wrote:
> Hi Suman,
>
> [please limit replies to not more than 80 columns per line]
>
> On 24 April 2013 00:50, Anna, Suman<s-anna@ti.com>  wrote:
>
>>>
>>> Documentation wise, we'd need documentation for what we finally wanna have,
>>> not the current implementation.
>>>
>>> There are also some desired features in a good API:-
>>>
>>> a) The API should be simple and efficient, i.e, submission of requests by clients
>>> and notifications of data received by the API should be possible from atomic
>>> context - most of the IPC is going to be about exchanging 'commands'. The API
>>> shouldn't add to the latencies.
>>
>> I think we will have need for both types. It really depends on the purpose of the IPC h/w - some would be simple messaging and may not need the atomic constraints. Sure enough, atomic context is the stricter one, so we can add that. I don't see any users of it currently though.
>>
> IMHO the API authors should take the other POV :  if _majority_ of
> clients _require_ non-atomic context, only then the API should support
> non-atomic version of the calls too. Otherwise let the minority of
> drivers schedule their sleepable tasks from the API's atomic
> callbacks.
>   The non-atomic API falls flat should just a single client comes with
> very low latency requirements.

In fact there are different situations for the non/atomic requirements 
(at least from ST POV)
- one mailbox could be non-atomic only (blocking if we reuse notifier 
wording)
- one mailbox could be atomic only
- one mailbox could be both depending on the context and usage (example: 
in the case of ST, one mailbox is used to access the analogue baseband, 
there is no particular atomic requirement for that except during 
register dump called form panic context)

For the 3rd point I agree with you, it should be considered as atomic 
and up to driver to schedule a sleepable task...

I have an internal implementation in which each mailbox is declared 
either atomic or blocking.
Depending on this parameter, the behavior of the mbox_send_mbox is 
changed...

>
>>>   For example, API should assume the IPC controller can detect and report when
>>> the remote asserts Ready-To-Receive (RTR). This is when the API callbacks
>>> .tx_done(mssg) on the client. If some h/w doesn't have RTR assertion interrupt,
>>> the API should provide optional feature to poll the controller periodically.
>>
>> Some of these are already part of the mailbox ops. FWIW, I don?t see a need for a .tx_done callback, as this state can really be maintained by the driver implementation itself. The mailbox_msg_send can return an error appropriately based on whether the h/w is ready or not. The current code has the support for queuing up a certain number of messages in case the h/w is busy, but I am planning to change this to a device-based property so that they can choose if it needs that support or not.
>>
> Not really. The TI's api does buffer requests from a client but it
> does not tell the client when the message is actually sent to the
> remote over the h/w channel.
>
>   Consider this example for better understanding, I must send a
> particular packet to the remote before I could, say, shutdown
> properly. The client calls mailbox_msg_send which queues the packet
> because the h/w was busy. Since the api does not inform the client
> about packet transmission, the client does not know when to shutdown
> (the packet may have been sent instantly or it may lie on the queue
> for a long time).
>   How do you fix this without tx_done callback method?
>
This mechanism was preserved to be compatibility with TI DSP bridge. But 
my initial proposal was to remove it, simply because today all mailbox 
customers consider that message is sent or not after calling 
mbox_send_message function.

>
>>>
>>> (d) The 'struct mailbox_msg' should be just an opaque void* - the client/protocol
>>> driver typecasts to void* the IPC controller specific message structure. API
>>> shouldn't aim the impossible of providing a generic message format.
>>
>> The size parameter would still be needed. Depending on the h/w, it can be just an u32 or a series of bytes, and even in the latter case, it is not guaranteed that all messages transmitted will occupy the entire h/w shared memory data packet. I can see the current header field getting absorbed into the opaque void * structure for the ST mailbox driver. The size and ptr together provide a very generic message format.
>>
> I am not sure about it. The API has no purpose for the .size (it's
> between the controller and the client driver).
> Controllers that could send variable length packets could define the
> packets format for clients as
>   struct foo_packet {
>           int size;
>           .......
>   }
> and exchange as typecasted to void* via the API. And this is just one
> way of doing it.
> The point being, the API should only pass pointer to packet info
> between controller and client drivers.
No today the size is used to store the message in the fifo and that's 
done by the framework.
If you decide to remove the fifo mechanism inherit for TI legacy, in 
that case yes the mailbox framework doesn't matter about message format 
and size.
>
>>>
>>> As you see there would be not much of the present left eventually. So I wonder
>>> if should sculpt a new api out of TI's or start from scratch?
>>> One of my controllers is similar to 'pl320' (which you left out converting to the
>>> API). I am in process of implementing all this assuming it is OK to keep a controller
>>> out of this API :)  So maybe we can collaborate on a new implementation from
>>> scratch.
>>
>> This series missed the 3.9 merge window, and is currently slated for getting merged into 3.10. The PL320 made it into 3.9 itself (I wasn't aware of it until I saw it in mainline) and created the drivers/mailbox folder.  I would think it would be relatively straight-forward to adopt it to the mailbox API, as it has only 3 API. We should be doing incremental changes on top of this series, as most of the base API would still be the same. The current series is helping out with couple of efforts, the breaking up of the PRCMU code and on the multiplatform support for OMAP with mailbox enabled. We can definitely collaborate on the improvements. Andy Green would also be interested, as he is also looking into adopting the mailbox API.
>>
> I don't think it should into 3.10 just because it missed the 3.9
> window, when its quality is doubted.
> I am going to have to live with IPC controller code for sometime now,
> so I care that I build upon a sound API. And I am more than happy to
> help on a proper implementation.
>
I'm really happy to see some new mailbox framework clients coming with 
new requirements to improve the solution.
Just for the story, mailbox framework topic was addressed during Linaro 
Copenhagen Connect and at the time being it was decided to reuse TI 
solution in a first time and then to improve it...

Regards,
Loic

> Regards,
> Jassi

[PATCHv3 00/14] drivers: mailbox: framework creation

[Loic PALLARDY]

Hi Jassi,

On 04/24/2013 09:59 AM, Jassi Brar wrote:
> Hi Loic,
>
> On 24 April 2013 13:09, Loic PALLARDY<loic.pallardy@st.com>  wrote:
>> Hi Jassi, Suman,
>>
>> Yes, the xxx_no_irq API has been introduced to answer some STE
>> requirements. It must be possible to send some message under atomic
>> context for different reasons (latency, during idle/suspend procedures...).
>> This is not the best way to do, but the goal was to preserve TI legacy
>> in a first step. As explained by Suman, this patch series is coming from
>> the original TI mailbox framework and is modified step by step to fit
>> with all platforms.
>>
> IMHO a better way is to introduce a clean generically designed API and
> convert the existing drivers one at time. Let the TI drivers work as
> such for a while until someone converts them to the common API.
> Cloning and then complete organ transplantation seems a very
> inefficient way to have something new ;)
>
As explained in previous mail, it was not the decision taken during 
Linaro Copenhagen Connect to restart from scratch, mainly because only 
ST and TI were interested by such framework at the time being.

>>>>
>>>> (d) The 'struct mailbox_msg' should be just an opaque void* - the client/protocol
>>>> driver typecasts to void* the IPC controller specific message structure. API
>>>> shouldn't aim the impossible of providing a generic message format.
>>>
>>> The size parameter would still be needed. Depending on the h/w, it can be just an u32 or a series of bytes, and even in the latter case, it is not guaranteed that all messages transmitted will occupy the entire h/w shared memory data packet. I can see the current header field getting absorbed into the opaque void * structure for the ST mailbox driver. The size and ptr together provide a very generic message format.
>> That's something we discussed with Suman. The mailbox framework should
>> be independent from message format. Since mailbox could be base don a
>> shared mem or an hw fifo, message size is mandatory to transmit the
>> right number of data.
>>
> I too believe the "mailbox framework should be independent from
> message format" but _also_ that .size doesn't have to be a part of the
> standard format.
> Maybe it will help if I know what you guys mean by "shared mem" or an
> "hw fifo" mailbox?
>
Hw fifo means that you what a HW IP which contains x bytes fifo.
When you write one byte in the fifo, an interrupt is automatically 
raised and caught by coprocessor which should read fifo on its side to 
empty it. In that case, Tx RTR is mandatory.
Shared mem, means that mailbox is made up of a cross interrupt + a 
status register (may be optional) + a shared memory in which messages 
are exchanged.
After writing message in shared mem, CPU has to raise the cross 
interrupt itself. In case flow control is less important...

/Loic
> Thanks
> -Jassi

[PATCHv3 00/14] drivers: mailbox: framework creation

[Jassi Brar]

Hi -

On 24 April 2013 13:38, Loic PALLARDY <loic.pallardy@st.com> wrote:
> Hi Jassi,
>
> On 04/24/2013 06:39 AM, Jassi Brar wrote:

>>   The non-atomic API falls flat should just a single client comes with
>> very low latency requirements.
>
> In fact there are different situations for the non/atomic requirements
> (at least from ST POV)
> - one mailbox could be non-atomic only (blocking if we reuse notifier
> wording)
> - one mailbox could be atomic only
> - one mailbox could be both depending on the context and usage (example:
> in the case of ST, one mailbox is used to access the analogue baseband,
> there is no particular atomic requirement for that except during
> register dump called form panic context)
>
> For the 3rd point I agree with you, it should be considered as atomic
> and up to driver to schedule a sleepable task...
>
> I have an internal implementation in which each mailbox is declared
> either atomic or blocking.
> Depending on this parameter, the behavior of the mbox_send_mbox is
> changed...
>
I am only talking about the nature of the API, which I think should as
fast as possible by providing atomic functions (except
mailbox_get/put).
If a controller needs to do sleep-prone tasks, let it schedule those
from API's atomic callbacks.
If a client needs to blink leds after receiving/sending a message, let
it schedule a work for that from API's atomic callback.
Majority of scenarios will simply be a client populating a structure
and pass it onto the controller driver which only need to program a
few registers to put the message on the h/w link.


>>   Consider this example for better understanding, I must send a
>> particular packet to the remote before I could, say, shutdown
>> properly. The client calls mailbox_msg_send which queues the packet
>> because the h/w was busy. Since the api does not inform the client
>> about packet transmission, the client does not know when to shutdown
>> (the packet may have been sent instantly or it may lie on the queue
>> for a long time).
>>   How do you fix this without tx_done callback method?
>>
> This mechanism was preserved to be compatibility with TI DSP bridge. But
> my initial proposal was to remove it, simply because today all mailbox
> customers consider that message is sent or not after calling
> mbox_send_message function.
>
Yeah, I too sense a lot of badness is inherited.

>>>
>>> The size parameter would still be needed. Depending on the h/w, it can be just an u32 or a series of bytes, and even in the latter case, it is not guaranteed that all messages transmitted will occupy the entire h/w shared memory data packet. I can see the current header field getting absorbed into the opaque void * structure for the ST mailbox driver. The size and ptr together provide a very generic message format.
>>>
>> I am not sure about it. The API has no purpose for the .size (it's
>> between the controller and the client driver).
>> Controllers that could send variable length packets could define the
>> packets format for clients as
>>   struct foo_packet {
>>           int size;
>>           .......
>>   }
>> and exchange as typecasted to void* via the API. And this is just one
>> way of doing it.
>> The point being, the API should only pass pointer to packet info
>> between controller and client drivers.
> No today the size is used to store the message in the fifo and that's
> done by the framework.
> If you decide to remove the fifo mechanism inherit for TI legacy, in
> that case yes the mailbox framework doesn't matter about message format
> and size.
>
Of course I desperately want to get rid of the TI legacy stuff in the
common api, that will simply not work on many platforms. My 'shutdown'
example above may be real for some platform.

>>
> I'm really happy to see some new mailbox framework clients coming with
> new requirements to improve the solution.
> Just for the story, mailbox framework topic was addressed during Linaro
> Copenhagen Connect and at the time being it was decided to reuse TI
> solution in a first time and then to improve it...
>
At the time my idea of mailbox was simply a tin box with a lock and a slit.
On a serious note, someone very important pointed out that things
discussed during events like Linaro shouldn't be taken as the
consensus of the community. "We" should have put it up for discussion
on the mailing lists.

>> Maybe it will help if I know what you guys mean by "shared mem" or an
>> "hw fifo" mailbox?
>>
> Hw fifo means that you what a HW IP which contains x bytes fifo.
> When you write one byte in the fifo, an interrupt is automatically
> raised and caught by coprocessor which should read fifo on its side to
> empty it. In that case, Tx RTR is mandatory.
> Shared mem, means that mailbox is made up of a cross interrupt + a
> status register (may be optional) + a shared memory in which messages
> are exchanged.
> After writing message in shared mem, CPU has to raise the cross
> interrupt itself. In case flow control is less important...
>
Well, I have a yet another type of controller where the IRQ status
register provides the data to be exchanged. Anyways I still maintain
that all three(or any more) controller types need not bother the API
with the size of message. The size should be a mandatory member of the
controller specific packet format that will be used by the client to
exchange data with it.

Regards,
-Jassi

[PATCHv3 00/14] drivers: mailbox: framework creation

[Suman Anna]

Jassi,

On 04/24/2013 03:56 AM, Jassi Brar wrote:
> Hi -
> 
> On 24 April 2013 13:38, Loic PALLARDY <loic.pallardy@st.com> wrote:
>> Hi Jassi,
>>
>> On 04/24/2013 06:39 AM, Jassi Brar wrote:
> 
>>>   The non-atomic API falls flat should just a single client comes with
>>> very low latency requirements.
>>
>> In fact there are different situations for the non/atomic requirements
>> (at least from ST POV)
>> - one mailbox could be non-atomic only (blocking if we reuse notifier
>> wording)
>> - one mailbox could be atomic only
>> - one mailbox could be both depending on the context and usage (example:
>> in the case of ST, one mailbox is used to access the analogue baseband,
>> there is no particular atomic requirement for that except during
>> register dump called form panic context)
>>
>> For the 3rd point I agree with you, it should be considered as atomic
>> and up to driver to schedule a sleepable task...
>>
>> I have an internal implementation in which each mailbox is declared
>> either atomic or blocking.
>> Depending on this parameter, the behavior of the mbox_send_mbox is
>> changed...
>>
> I am only talking about the nature of the API, which I think should as
> fast as possible by providing atomic functions (except
> mailbox_get/put).
> If a controller needs to do sleep-prone tasks, let it schedule those
> from API's atomic callbacks.
> If a client needs to blink leds after receiving/sending a message, let
> it schedule a work for that from API's atomic callback.
> Majority of scenarios will simply be a client populating a structure
> and pass it onto the controller driver which only need to program a
> few registers to put the message on the h/w link.

I think there are two things here - one is what the client needs to do
upon sending/receiving a message, and the other is what the send API or
the mailbox controller should do when a client tried to send a message
and the controller's shared message/transport is not free (irrespective
of the atomic or non-atomic context). The kfifos in the common driver
code are currently solving the latter problem. The current send API
directly uses the controller to send if it is free, and uses buffering
only when it is busy. But, I see your point that this should should be
the responsibility of the specific controller, or even a property of the
specific mailbox belonging to that controller. This direction would put
most of the onus on the specific controller drivers, and probably the
main API would be very simple. Another factor / attribute to consider is
that a given mailbox may be sharable or exclusive, things are simple if
it is exclusive to a client driver.

> 
> 
>>>   Consider this example for better understanding, I must send a
>>> particular packet to the remote before I could, say, shutdown
>>> properly. The client calls mailbox_msg_send which queues the packet
>>> because the h/w was busy. Since the api does not inform the client
>>> about packet transmission, the client does not know when to shutdown
>>> (the packet may have been sent instantly or it may lie on the queue
>>> for a long time).
>>>   How do you fix this without tx_done callback method?
>>>
>> This mechanism was preserved to be compatibility with TI DSP bridge. But
>> my initial proposal was to remove it, simply because today all mailbox
>> customers consider that message is sent or not after calling
>> mbox_send_message function.
>>
> Yeah, I too sense a lot of badness is inherited.

We are talking two fundamentally different usecases/needs here,
depending on the type of the controller. You seem to be coming from a
usecase where the client driver needs to know when every message is
transmitted (from an atomic context, it is a moot point since either you
are successful or not when transmitting). The example problem you
mentioned maybe very relevant to some hardware, but it hadn't been an
issue on OMAP since the mailbox driver is just for sending messages and
not managing the state of the remote. The remote is a processor in OMAP,
and its state machine is controlled by the remoteproc driver. The remote
has to ack before it can be shutdown. I would imagine that getting a
.tx_done on a particular message is not good enough to know that the
remote is ready for shutdown. I can imagine it to be useful where there
is some inherent knowledge that the client needs to proceed with the
next steps when a message is sent. That said, we need to go with the
stricter one.

> 
>>>>
>>>> The size parameter would still be needed. Depending on the h/w, it can be just an u32 or a series of bytes, and even in the latter case, it is not guaranteed that all messages transmitted will occupy the entire h/w shared memory data packet. I can see the current header field getting absorbed into the opaque void * structure for the ST mailbox driver. The size and ptr together provide a very generic message format.
>>>>
>>> I am not sure about it. The API has no purpose for the .size (it's
>>> between the controller and the client driver).
>>> Controllers that could send variable length packets could define the
>>> packets format for clients as
>>>   struct foo_packet {
>>>           int size;
>>>           .......
>>>   }
>>> and exchange as typecasted to void* via the API. And this is just one
>>> way of doing it.
>>> The point being, the API should only pass pointer to packet info
>>> between controller and client drivers.
>> No today the size is used to store the message in the fifo and that's
>> done by the framework.
>> If you decide to remove the fifo mechanism inherit for TI legacy, in
>> that case yes the mailbox framework doesn't matter about message format
>> and size.
>>

Yep, the size field was a direct need from the buffering needs if the
controller were busy. The Tx RTR in fact was used to flush out the
buffered messages.

> Of course I desperately want to get rid of the TI legacy stuff in the
> common api, that will simply not work on many platforms. My 'shutdown'
> example above may be real for some platform.
> 
>>>
>> I'm really happy to see some new mailbox framework clients coming with
>> new requirements to improve the solution.
>> Just for the story, mailbox framework topic was addressed during Linaro
>> Copenhagen Connect and at the time being it was decided to reuse TI
>> solution in a first time and then to improve it...
>>
> At the time my idea of mailbox was simply a tin box with a lock and a slit.
> On a serious note, someone very important pointed out that things
> discussed during events like Linaro shouldn't be taken as the
> consensus of the community. "We" should have put it up for discussion
> on the mailing lists.
> 
>>> Maybe it will help if I know what you guys mean by "shared mem" or an
>>> "hw fifo" mailbox?
>>>
>> Hw fifo means that you what a HW IP which contains x bytes fifo.
>> When you write one byte in the fifo, an interrupt is automatically
>> raised and caught by coprocessor which should read fifo on its side to
>> empty it. In that case, Tx RTR is mandatory.
>> Shared mem, means that mailbox is made up of a cross interrupt + a
>> status register (may be optional) + a shared memory in which messages
>> are exchanged.
>> After writing message in shared mem, CPU has to raise the cross
>> interrupt itself. In case flow control is less important...
>>
> Well, I have a yet another type of controller where the IRQ status
> register provides the data to be exchanged. Anyways I still maintain
> that all three(or any more) controller types need not bother the API
> with the size of message. The size should be a mandatory member of the
> controller specific packet format that will be used by the client to
> exchange data with it.

I think the discussion on size field is mostly semantics, since it would
be very much relevant if the controller's transport payload is more than
the size of a standard type. It is a non-factor for controllers whose
payload size is a standard type (like for OMAP, where the msg size is
u32). Having 32 bytes of payload, for example, almost always means your
controller packet has to have a size field indicating the actual number
of bytes in the message, and it doesn't matter whether the size is a
register or has to exchanged with the remote as part of the transport
packet structure. My only concern here is that if there can be multiple
clients for a particular mailbox/controller, then all the clients would
have to have an agreement on the controller packet type, and the clients
would mostly have to include the standard mailbox.h as well as a
controller-specific header.

Overall, I see it coming down to following points:
1. Calling contexts: Atomic & non-atomic contexts, with the latter
becoming an extension of the atomic case. I guess this mainly goes with
the controller functional integration - whether it is used for
non-urgent messaging or h/w controller command messages (like PM
usecases?) where .tx_done is relevant.
2. Behavior of the API or controller driver if the controller transport
is busy.
3. Shareable/exclusive nature of a mailbox. If it is shareable, then
duplicating the behavior between clients is not worth it, and this
should be absorbed into the respective controller driver.

regards
Suman

[PATCHv3 00/14] drivers: mailbox: framework creation

[Jassi Brar]

On 25 April 2013 04:46, Suman Anna <s-anna@ti.com> wrote:
> On 04/24/2013 03:56 AM, Jassi Brar wrote:
>

> I think there are two things here - one is what the client needs to do
> upon sending/receiving a message, and the other is what the send API or
> the mailbox controller should do when a client tried to send a message
> and the controller's shared message/transport is not free (irrespective
> of the atomic or non-atomic context). The kfifos in the common driver
> code are currently solving the latter problem. The current send API
> directly uses the controller to send if it is free, and uses buffering
> only when it is busy. But, I see your point that this should should be
> the responsibility of the specific controller, or even a property of the
> specific mailbox belonging to that controller. This direction would put
> most of the onus on the specific controller drivers, and probably the
> main API would be very simple. Another factor / attribute to consider is
> that a given mailbox may be sharable or exclusive, things are simple if
> it is exclusive to a client driver.
>
I never suggested we don't use buffering. I too believe the API should
buffer requests but also that it should still do atomic callbacks. The
impact on implementation would be that the queue buffer can not grow
at runtime. But that should be fine because a reasonable size (say 10
or even 50) could be chosen and we allow submission of requests from
tx_done callback.

>
> We are talking two fundamentally different usecases/needs here,
> depending on the type of the controller. You seem to be coming from a
> usecase where the client driver needs to know when every message is
> transmitted (from an atomic context, it is a moot point since either you
> are successful or not when transmitting).
>
I am afraid you are confusing the meaning of 'atomic context' here.
atomic context doesn't mean instant transmission of data, but that the
API calls could be made from even atomic context and that the client &
controller can't sleep in callbacks from the API. So it's not moot.

> The remote
> has to ack before it can be shutdown. I would imagine that getting a
> .tx_done on a particular message is not good enough to know that the
> remote is ready for shutdown. I can imagine it to be useful where there
> is some inherent knowledge that the client needs to proceed with the
> next steps when a message is sent.
>
Of course we are not specifying how the mailbox signals are
interpreted by the remote. It should suffice just to realize that
there exists genuine requirement for a client to know when its message
was received by the remote.

> That said, we need to go with the
> stricter one.
>
Great, that we agree.

> My only concern here is that if there can be multiple
> clients for a particular mailbox/controller, then all the clients would
> have to have an agreement on the controller packet type, and the clients
> would mostly have to include the standard mailbox.h as well as a
> controller-specific header.
>
It's the controller driver that actually puts the data on the bus. So
only it should define the format in which it accepts data from the
clients. Every client should simply populate the packet structure
defined in  my_lovely_controller.h and pass on the struct pointer to
the controller driver via API.
No negotiations for the driver seat among passengers :)

> Overall, I see it coming down to following points:
> 1. Calling contexts: Atomic & non-atomic contexts, with the latter
> becoming an extension of the atomic case. I guess this mainly goes with
> the controller functional integration - whether it is used for
> non-urgent messaging or h/w controller command messages (like PM
> usecases?) where .tx_done is relevant.
>
Urgency or not is a business of the client driver. The API and the
controller driver should not delay things more than absolute
necessary.
.tx_done is not about urgency but about h/w provision of 'ack'.

> 2. Behavior of the API or controller driver if the controller transport
> is busy.
I think we both want requests buffered in the API.

> 3. Shareable/exclusive nature of a mailbox. If it is shareable, then
> duplicating the behavior between clients is not worth it, and this
> should be absorbed into the respective controller driver.
>
I think the mailbox should be exclusively held by a client. That makes
many things simpler. Also remote firmwares won't be always robust
enough to handle commands from different subsystems intermixed. The
API only has to make sure the mailbox_get/put operations are very
thin.

Regards,
Jassi

[PATCHv3 00/14] drivers: mailbox: framework creation

[Suman Anna]

Jassi,

On 04/25/2013 12:20 AM, Jassi Brar wrote:
> On 25 April 2013 04:46, Suman Anna <s-anna@ti.com> wrote:
>> On 04/24/2013 03:56 AM, Jassi Brar wrote:
>>
> 
>> I think there are two things here - one is what the client needs to do
>> upon sending/receiving a message, and the other is what the send API or
>> the mailbox controller should do when a client tried to send a message
>> and the controller's shared message/transport is not free (irrespective
>> of the atomic or non-atomic context). The kfifos in the common driver
>> code are currently solving the latter problem. The current send API
>> directly uses the controller to send if it is free, and uses buffering
>> only when it is busy. But, I see your point that this should should be
>> the responsibility of the specific controller, or even a property of the
>> specific mailbox belonging to that controller. This direction would put
>> most of the onus on the specific controller drivers, and probably the
>> main API would be very simple. Another factor / attribute to consider is
>> that a given mailbox may be sharable or exclusive, things are simple if
>> it is exclusive to a client driver.
>>
> I never suggested we don't use buffering. I too believe the API should
> buffer requests but also that it should still do atomic callbacks. The
> impact on implementation would be that the queue buffer can not grow
> at runtime. But that should be fine because a reasonable size (say 10
> or even 50) could be chosen and we allow submission of requests from
> tx_done callback.

Yeah, even the current kfifo approach doesn't grow the queue buffer at
runtime, and the size of the kfifo is determined at driver init time
based on the controller. If the queue is also full, then you fail
tranmitting right away. OK, I thought you didn't want buffering, if that
is not the case, then the buffering should be within the main driver
code, like it is now, but configurable based on the controller or
mailbox properties. If it is present in individual controller drivers,
then we would be duplicating stuff. Are you envisioning that this be
left to the individual controllers?

> 
>>
>> We are talking two fundamentally different usecases/needs here,
>> depending on the type of the controller. You seem to be coming from a
>> usecase where the client driver needs to know when every message is
>> transmitted (from an atomic context, it is a moot point since either you
>> are successful or not when transmitting).
>>
> I am afraid you are confusing the meaning of 'atomic context' here.
> atomic context doesn't mean instant transmission of data, but that the
> API calls could be made from even atomic context and that the client &
> controller can't sleep in callbacks from the API. So it's not moot.

I understood the atomic context, and the question is about the behavior
of the '.tx_done' callback when sending from atomic context. Is there
such a usecase/need for you in that you want to send a response back
from an atomic context, yet get a callback?

> 
>> The remote
>> has to ack before it can be shutdown. I would imagine that getting a
>> .tx_done on a particular message is not good enough to know that the
>> remote is ready for shutdown. I can imagine it to be useful where there
>> is some inherent knowledge that the client needs to proceed with the
>> next steps when a message is sent.
>>
> Of course we are not specifying how the mailbox signals are
> interpreted by the remote. It should suffice just to realize that
> there exists genuine requirement for a client to know when its message
> was received by the remote.
> 
>> That said, we need to go with the
>> stricter one.
>>
> Great, that we agree.
> 
>> My only concern here is that if there can be multiple
>> clients for a particular mailbox/controller, then all the clients would
>> have to have an agreement on the controller packet type, and the clients
>> would mostly have to include the standard mailbox.h as well as a
>> controller-specific header.
>>
> It's the controller driver that actually puts the data on the bus. So
> only it should define the format in which it accepts data from the
> clients. Every client should simply populate the packet structure
> defined in  my_lovely_controller.h and pass on the struct pointer to
> the controller driver via API.
> No negotiations for the driver seat among passengers :)

OK, I was trying to avoid including my_lovely_controller.h and only
include the standard .h file as a client user, the client would anyway
need to have the intrinsic knowledge of the packet structure. And if you
were to do buffering in the common driver code, you would need the size
field outside. The void *msg packet structure would still be an
understanding between the client and the controller. So, it is a
tradeoff between just using void * and leave the buffering to controller
driver (potential duplication) & using a size and void *, along with
buffering in common driver code.

> 
>> Overall, I see it coming down to following points:
>> 1. Calling contexts: Atomic & non-atomic contexts, with the latter
>> becoming an extension of the atomic case. I guess this mainly goes with
>> the controller functional integration - whether it is used for
>> non-urgent messaging or h/w controller command messages (like PM
>> usecases?) where .tx_done is relevant.
>>
> Urgency or not is a business of the client driver. The API and the
> controller driver should not delay things more than absolute
> necessary.
> .tx_done is not about urgency but about h/w provision of 'ack'.
> 
>> 2. Behavior of the API or controller driver if the controller transport
>> is busy.
> I think we both want requests buffered in the API.
> 
>> 3. Shareable/exclusive nature of a mailbox. If it is shareable, then
>> duplicating the behavior between clients is not worth it, and this
>> should be absorbed into the respective controller driver.
>>
> I think the mailbox should be exclusively held by a client. That makes
> many things simpler. Also remote firmwares won't be always robust
> enough to handle commands from different subsystems intermixed. The
> API only has to make sure the mailbox_get/put operations are very
> thin.

This might be the case for specific remotes where we expect only one
client driver to be responsible for talking to it, but for generic
offloading, you do not want to have this restriction. You do not want
peer clients to go through a single main client, as the latencies or the
infrastructure imposed by the main client may not be suitable for the
other clients. The stricter usecase here would be the shareable mailbox,
and if it is exclusive, as dictated by a controller or device property,
then so be it and things would get simplified for that controller/device.

regards
Suman

[PATCHv3 00/14] drivers: mailbox: framework creation

[Andy Green]

On 26/04/13 06:29, the mail apparently from Suman Anna included:

Hi -

>>> 3. Shareable/exclusive nature of a mailbox. If it is shareable, then
>>> duplicating the behavior between clients is not worth it, and this
>>> should be absorbed into the respective controller driver.
>>>
>> I think the mailbox should be exclusively held by a client. That makes
>> many things simpler. Also remote firmwares won't be always robust
>> enough to handle commands from different subsystems intermixed. The
>> API only has to make sure the mailbox_get/put operations are very
>> thin.
>
> This might be the case for specific remotes where we expect only one
> client driver to be responsible for talking to it, but for generic
> offloading, you do not want to have this restriction. You do not want
> peer clients to go through a single main client, as the latencies or the
> infrastructure imposed by the main client may not be suitable for the
> other clients. The stricter usecase here would be the shareable mailbox,
> and if it is exclusive, as dictated by a controller or device property,
> then so be it and things would get simplified for that controller/device.

Knowing why Jassi mentioned this, the situation is a bit different than 
what you replied to.  There are in fact multiple client drivers that can 
asynchronously decide to initiate communication on the same mailbox. 
Some of the client need to perform multi-step sequencing and lock the 
mailbox for the duration.

Right now we can implement that by having a driver on top to mediate, 
Jassi is suggesting being able to do the client locking at your layer as 
a primitive will simplify things, not least get rid of the mediation 
driver.  Your layer has concept of completion and notifier already so it 
seems it wouldn't take much more.

-Andy

-- 
Andy Green | Fujitsu Landing Team Leader
Linaro.org ? Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  - 
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog

[PATCHv3 00/14] drivers: mailbox: framework creation

[Jassi Brar]

Hi Suman,

On 26 April 2013 03:59, Suman Anna <s-anna@ti.com> wrote:
> On 04/25/2013 12:20 AM, Jassi Brar wrote:
> tranmitting right away. OK, I thought you didn't want buffering, if that
> is not the case, then the buffering should be within the main driver
> code, like it is now, but configurable based on the controller or
> mailbox properties. If it is present in individual controller drivers,
> then we would be duplicating stuff. Are you envisioning that this be
> left to the individual controllers?
>
Please don't accuse me of such bad visions :)
I never said no-buffering and I never said buffering should be in
controller drivers. In fact I don't remember ever objecting to how
buffering is done in TI's framework.
A controller could service only 1 request at a time so lets give it
just 1 at a time. Let the API handle the complexity of buffering.

>> I am afraid you are confusing the meaning of 'atomic context' here.
>> atomic context doesn't mean instant transmission of data, but that the
>> API calls could be made from even atomic context and that the client &
>> controller can't sleep in callbacks from the API. So it's not moot.
>
> I understood the atomic context, and the question is about the behavior
> of the '.tx_done' callback when sending from atomic context. Is there
> such a usecase/need for you in that you want to send a response back
> from an atomic context, yet get a callback?
>
Let me get in detail...
The TX-Wheel has to tick. Someone has to tell the framework that the
last TX was consumed by the remote and now it's time to submit the
next TX (RX will always be driven by the controller's IRQ so it's
straight).
If the controller h/w gets some interrupt indicating
Remote-RTR/TX-Done then the ticker is driven by controller's TX-IRQ
handler. Otherwise, if the controller does sense RTR but not report
(by reading status in some register but no irq), then API has to poll
it periodically and move the ticker. If the controller can neither
report nor sense RTR, the client/protocol driver must run the ticker
(usually upon receiving some ACK packet on the RX channel).
This TX ticker should be callable from atomic context (controller's
IRQ handler) and calls into callback of the client. It is desirable
that the client be able to submit yet another TX request from the
callback. That way the client can avoid having to schedule work from
the callback if the TX doesn't involve any sleepable task. The scheme
is working very well in DMA-Engine stack.

BTW, TI's RX mechanism too seems broken for common API. Receiving
every few bytes via 'notify' mechanism is very inefficient. Imagine a
platform with no shared memory between co-processors and the local
wants to diagnose the remote by asking critical data at least KBs in
size.
 So when API has nothing to do with received packet and the controller
has to get rid of it asap so as to be able to receive the next, IMHO
there should be short-circuit from controller to client via the API.
No delay, no buffering of RX.


>> It's the controller driver that actually puts the data on the bus. So
>> only it should define the format in which it accepts data from the
>> clients. Every client should simply populate the packet structure
>> defined in  my_lovely_controller.h and pass on the struct pointer to
>> the controller driver via API.
>> No negotiations for the driver seat among passengers :)
>
> OK, I was trying to avoid including my_lovely_controller.h and only
> include the standard .h file as a client user, the client would anyway
> need to have the intrinsic knowledge of the packet structure.
>
Not including my_controller.h doesn't make things standard.
As we know, the client anyway has to have intrinsic knowledge of the
packet structure(which is dictated by the controller), so not
including my_controller.h will only confuse people as to where the
packet info came from?

>>>
>> I think the mailbox should be exclusively held by a client. That makes
>> many things simpler. Also remote firmwares won't be always robust
>> enough to handle commands from different subsystems intermixed. The
>> API only has to make sure the mailbox_get/put operations are very
>> thin.
>
> This might be the case for specific remotes where we expect only one
> client driver to be responsible for talking to it, but for generic
> offloading, you do not want to have this restriction. You do not want
> peer clients to go through a single main client, as the latencies or the
> infrastructure imposed by the main client may not be suitable for the
> other clients. The stricter usecase here would be the shareable mailbox,
> and if it is exclusive, as dictated by a controller or device property,
> then so be it and things would get simplified for that controller/device.
>
Shared Vs Exclusive had been the dilemma of DMAEngine too.
If the controller has physical channels at least as many as clients,
exclusivity is no problem.
Sharing is desirable when the controller has to serve clients more
than its physical channels. We solve that by having the controller
declare exclusive virtual channels and internally scheduling the
requests onto physical channels.
 And as Andy pointed out, some remote-ends may not cope with requests
coming from different subsystems intermixed.

Regards,
-Jassi

[PATCHv3 00/14] drivers: mailbox: framework creation

[Suman Anna]

Hi Jassi,

On 04/25/2013 10:46 PM, Jassi Brar wrote:
> Hi Suman,
> 
> On 26 April 2013 03:59, Suman Anna <s-anna@ti.com> wrote:
>> On 04/25/2013 12:20 AM, Jassi Brar wrote:
>> tranmitting right away. OK, I thought you didn't want buffering, if that
>> is not the case, then the buffering should be within the main driver
>> code, like it is now, but configurable based on the controller or
>> mailbox properties. If it is present in individual controller drivers,
>> then we would be duplicating stuff. Are you envisioning that this be
>> left to the individual controllers?
>>
> Please don't accuse me of such bad visions :)
> I never said no-buffering and I never said buffering should be in
> controller drivers. In fact I don't remember ever objecting to how
> buffering is done in TI's framework.
> A controller could service only 1 request at a time so lets give it
> just 1 at a time. Let the API handle the complexity of buffering.
> 

Alright, guess this got lost in translation :). I interpreted based on
the fact that you wanted to get rid of the size field from the
mailbox_msg definition. Do you have a different mechanism in mind for
the buffering compared to the present one?

>>> I am afraid you are confusing the meaning of 'atomic context' here.
>>> atomic context doesn't mean instant transmission of data, but that the
>>> API calls could be made from even atomic context and that the client &
>>> controller can't sleep in callbacks from the API. So it's not moot.
>>
>> I understood the atomic context, and the question is about the behavior
>> of the '.tx_done' callback when sending from atomic context. Is there
>> such a usecase/need for you in that you want to send a response back
>> from an atomic context, yet get a callback?
>>
> Let me get in detail...
> The TX-Wheel has to tick. Someone has to tell the framework that the
> last TX was consumed by the remote and now it's time to submit the
> next TX (RX will always be driven by the controller's IRQ so it's
> straight).
> If the controller h/w gets some interrupt indicating
> Remote-RTR/TX-Done then the ticker is driven by controller's TX-IRQ
> handler. Otherwise, if the controller does sense RTR but not report
> (by reading status in some register but no irq), then API has to poll
> it periodically and move the ticker. If the controller can neither
> report nor sense RTR, the client/protocol driver must run the ticker
> (usually upon receiving some ACK packet on the RX channel).

OK, I didn't think of a no RTR interrupt-based controller. I would thing
that such a controller is very rudimentary. I wonder if there are any
controllers like this out there.

> This TX ticker should be callable from atomic context (controller's
> IRQ handler) and calls into callback of the client. It is desirable
> that the client be able to submit yet another TX request from the
> callback. That way the client can avoid having to schedule work from
> the callback if the TX doesn't involve any sleepable task. The scheme
> is working very well in DMA-Engine stack.
> 
> BTW, TI's RX mechanism too seems broken for common API. Receiving
> every few bytes via 'notify' mechanism is very inefficient. Imagine a
> platform with no shared memory between co-processors and the local
> wants to diagnose the remote by asking critical data at least KBs in
> size.

No shared memory between co-processors and a relatively slow wire
transport is a bad architecture design to begin with.

>  So when API has nothing to do with received packet and the controller
> has to get rid of it asap so as to be able to receive the next, IMHO
> there should be short-circuit from controller to client via the API.
> No delay, no buffering of RX.

The current TI design is based on the fact that we can get multiple
messages on a single interrupt due to the h/w fifo and the driver takes
care of the bottom-half. Leaving it to the client is putting a lot of
faith in the client and doesn't scale to multiple clients. The client
would have to perform mostly the same as the driver is doing - so this
goes back to the base discussion point that we have - which is the lack
of support for atomic_context receivers in the current code. I perceive
this as an attribute of the controller/mailbox device itself rather than
the client.

> 
> 
>>> It's the controller driver that actually puts the data on the bus. So
>>> only it should define the format in which it accepts data from the
>>> clients. Every client should simply populate the packet structure
>>> defined in  my_lovely_controller.h and pass on the struct pointer to
>>> the controller driver via API.
>>> No negotiations for the driver seat among passengers :)
>>
>> OK, I was trying to avoid including my_lovely_controller.h and only
>> include the standard .h file as a client user, the client would anyway
>> need to have the intrinsic knowledge of the packet structure.
>>
> Not including my_controller.h doesn't make things standard.
> As we know, the client anyway has to have intrinsic knowledge of the
> packet structure(which is dictated by the controller), so not
> including my_controller.h will only confuse people as to where the
> packet info came from?

OK, agreed.

> 
>>>>
>>> I think the mailbox should be exclusively held by a client. That makes
>>> many things simpler. Also remote firmwares won't be always robust
>>> enough to handle commands from different subsystems intermixed. The
>>> API only has to make sure the mailbox_get/put operations are very
>>> thin.
>>
>> This might be the case for specific remotes where we expect only one
>> client driver to be responsible for talking to it, but for generic
>> offloading, you do not want to have this restriction. You do not want
>> peer clients to go through a single main client, as the latencies or the
>> infrastructure imposed by the main client may not be suitable for the
>> other clients. The stricter usecase here would be the shareable mailbox,
>> and if it is exclusive, as dictated by a controller or device property,
>> then so be it and things would get simplified for that controller/device.
>>
> Shared Vs Exclusive had been the dilemma of DMAEngine too.
> If the controller has physical channels at least as many as clients,
> exclusivity is no problem.
> Sharing is desirable when the controller has to serve clients more
> than its physical channels. We solve that by having the controller
> declare exclusive virtual channels and internally scheduling the
> requests onto physical channels.
>  And as Andy pointed out, some remote-ends may not cope with requests
> coming from different subsystems intermixed.

Even though both the scenarios look very similar, I believe there are
some slight differences. All the devices belonging to a controller may
not be of the same type (meaning, intended towards the same remote or be
used interchangeably with one another). It is definitely possible if you
have a similar scenario to the DMA physical channels and your remote
rx interrupt can identify the device/channel to process. This would be
very much dependent on the architecture of a controller. The particular
example that I have in mind is s/w clients between the same set of
remote and host entities using the same device - the send part is anyway
arbitrated by the controller, and the same received message can be
delivered to the clients, with the clients making the decision whether
the packet belongs to them or not. I agree that all remote-ends will not
be able to cope up intermixed requests, but isn't this again a
controller architecture dependent?

regards
Suman

[PATCHv3 00/14] drivers: mailbox: framework creation

[Andy Green]

On 27/04/13 09:04, the mail apparently from Suman Anna included:

Hi Suman -

> Even though both the scenarios look very similar, I believe there are
> some slight differences. All the devices belonging to a controller may
> not be of the same type (meaning, intended towards the same remote or be
> used interchangeably with one another). It is definitely possible if you
> have a similar scenario to the DMA physical channels and your remote
> rx interrupt can identify the device/channel to process. This would be
> very much dependent on the architecture of a controller. The particular
> example that I have in mind is s/w clients between the same set of
> remote and host entities using the same device - the send part is anyway
> arbitrated by the controller, and the same received message can be
> delivered to the clients, with the clients making the decision whether
> the packet belongs to them or not. I agree that all remote-ends will not
> be able to cope up intermixed requests, but isn't this again a
> controller architecture dependent?

Maybe it's helpful to describe our situation more concretely, because 
the problem is not coming from "the architecture of the [mailbox] 
controller".

In the SoC we work on clock and subsystem power control registers, a 
serial bus, and some other assets are not directly accessible from 
Linux.  We must ask a coprocessor to operate these for us, using the 
mailbox.

So at any one time, the clock driver or voltagedomain driver for the SoC 
may want to own the mailbox and perform one or more operations over it 
synchronously, in some cases on the remote side involving transactions 
on a serial bus.  We don't want other transactions to be occurring while 
we wait for the serial bus to complete what the driver who started that 
asked for, for example.

We can cope with this by having an outer driver mediate access to the 
mailbox.  But then there are multiple sync primitives like completions 
and notifiers per operation, because your core already does this.

In short the FIFO + sync operations approach your core implements 
doesn't fit our use case.  That can be our problem, in which case we'll 
live with the outer mediation driver on top of the mailbox, or it can be 
a sign the fixed choice of FIFO + sync operations in your core did not 
quite hit the nail on the head to really model all the facets of legit 
mailbox usage.

At least, this real scenario should be interesting to think about before 
rejecting ^^

-Andy

-- 
Andy Green | Fujitsu Landing Team Leader
Linaro.org ? Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  - 
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog

[PATCHv3 00/14] drivers: mailbox: framework creation

[Jassi Brar]

Hi Suman,

>> On 26 April 2013 03:59, Suman Anna <s-anna@ti.com> wrote:
>>> On 04/25/2013 12:20 AM, Jassi Brar wrote:

>> I never said no-buffering and I never said buffering should be in
>> controller drivers. In fact I don't remember ever objecting to how
>> buffering is done in TI's framework.
>> A controller could service only 1 request at a time so lets give it
>> just 1 at a time. Let the API handle the complexity of buffering.
>>
>
> Alright, guess this got lost in translation :). I interpreted based on
> the fact that you wanted to get rid of the size field from the
> mailbox_msg definition. Do you have a different mechanism in mind for
> the buffering compared to the present one?
>
Sure, a very simple but efficient one. I had started on pseudo code
implementation the day I first replied, but now I have real code with
the PL320 controller and the Highbank client converted to the API. All
that I say features in the new design. Polishing and documentation
will take just a few hours more. You could see end to end what I have
been talking about.


>
> OK, I didn't think of a no RTR interrupt-based controller. I would thing
> that such a controller is very rudimentary. I wonder if there are any
> controllers like this out there.
>
One of my controllers is like that :)

>>
>> BTW, TI's RX mechanism too seems broken for common API. Receiving
>> every few bytes via 'notify' mechanism is very inefficient. Imagine a
>> platform with no shared memory between co-processors and the local
>> wants to diagnose the remote by asking critical data at least KBs in
>> size.
>
> No shared memory between co-processors and a relatively slow wire
> transport is a bad architecture design to begin with.
>
IMHO it's only about private memory. Even if the controller transfers,
say, 10bytes/interrupt there could always be a requirement to read
some 1MB region of remote's private memory. And the same logic implies
that our TX too should be as fast as possible - the remote might need
its 1MB firmware over the link. So let us just try to serve all
designs rather than evaluate them :)


>>  So when API has nothing to do with received packet and the controller
>> has to get rid of it asap so as to be able to receive the next, IMHO
>> there should be short-circuit from controller to client via the API.
>> No delay, no buffering of RX.
>
> The current TI design is based on the fact that we can get multiple
> messages on a single interrupt due to the h/w fifo and the driver takes
> care of the bottom-half. Leaving it to the client is putting a lot of
> faith in the client and doesn't scale to multiple clients. The client
> would have to perform mostly the same as the driver is doing - so this
> goes back to the base discussion point that we have - which is the lack
> of support for atomic_context receivers in the current code. I perceive
> this as an attribute of the controller/mailbox device itself rather than
> the client.
>
Sorry, I don't understand the concern about faith.
If the controller h/w absolutely can not tell the remote(sender) of a
received packet (as seems to be your case), its driver shouldn't even
try to demux the received messages. The client driver must know which
remotes could send it a message and how to discern them on the
platform. Some 'server' RX client is needed here.
If the controller could indeed map received packet onto remotes, then
ideally the controller driver should declare one (RX only) channel for
each such remote and demux packets onto them.
In either case, 'notify' mechanism is not necessary.


> I agree that all remote-ends will not
> be able to cope up intermixed requests, but isn't this again a
> controller architecture dependent?
>
I think it's more about remote's protocol implementation than
controller's architecture.
If tomorrow TI's remote firmware introduces a new set of critical
commands that may arrive only in a particular sequence, you'll find
yourself sharing a ride on our dinghy :)
And Andy already explained where we come from.

Regards,
-Jassi

[PATCH 1/3] mailbox: rename pl320-ipc specific mailbox.h

[jaswinder.singh at linaro.org]

From: Suman Anna <s-anna@ti.com>

The patch 30058677 "ARM / highbank: add support for pl320 IPC"
added a pl320 IPC specific header file as a generic mailbox.h.
This file has been renamed appropriately to allow the
introduction of the generic mailbox API framework.

Signed-off-by: Suman Anna <s-anna@ti.com>
Cc: Mark Langsdorf <mark.langsdorf@calxeda.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/highbank-cpufreq.c       |    2 +-
 drivers/mailbox/pl320-ipc.c              |    2 +-
 include/linux/{mailbox.h => pl320-ipc.h} |    0
 3 files changed, 2 insertions(+), 2 deletions(-)
 rename include/linux/{mailbox.h => pl320-ipc.h} (100%)

diff --git a/drivers/cpufreq/highbank-cpufreq.c b/drivers/cpufreq/highbank-cpufreq.c
index b61b5a3..3118b87 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/mailbox.h>
+#include <linux/pl320-ipc.h>
 #include <linux/platform_device.h>
 
 #define HB_CPUFREQ_CHANGE_NOTE	0x80000001
diff --git a/drivers/mailbox/pl320-ipc.c b/drivers/mailbox/pl320-ipc.c
index d873cba..f3755e0 100644
--- a/drivers/mailbox/pl320-ipc.c
+++ b/drivers/mailbox/pl320-ipc.c
@@ -26,7 +26,7 @@
 #include <linux/device.h>
 #include <linux/amba/bus.h>
 
-#include <linux/mailbox.h>
+#include <linux/pl320-ipc.h>
 
 #define IPCMxSOURCE(m)		((m) * 0x40)
 #define IPCMxDSET(m)		(((m) * 0x40) + 0x004)
diff --git a/include/linux/mailbox.h b/include/linux/pl320-ipc.h
similarity index 100%
rename from include/linux/mailbox.h
rename to include/linux/pl320-ipc.h
-- 
1.7.10.4

[PATCH 1/3] mailbox: rename pl320-ipc specific mailbox.h

[Mark Langsdorf]

On 04/27/2013 01:05 PM, jaswinder.singh at linaro.org wrote:
> From: Suman Anna <s-anna@ti.com>
> 
> The patch 30058677 "ARM / highbank: add support for pl320 IPC"
> added a pl320 IPC specific header file as a generic mailbox.h.
> This file has been renamed appropriately to allow the
> introduction of the generic mailbox API framework.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>
> Cc: Mark Langsdorf <mark.langsdorf@calxeda.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/highbank-cpufreq.c       |    2 +-
>  drivers/mailbox/pl320-ipc.c              |    2 +-
>  include/linux/{mailbox.h => pl320-ipc.h} |    0
>  3 files changed, 2 insertions(+), 2 deletions(-)
>  rename include/linux/{mailbox.h => pl320-ipc.h} (100%)
> 
> diff --git a/drivers/cpufreq/highbank-cpufreq.c b/drivers/cpufreq/highbank-cpufreq.c
> index b61b5a3..3118b87 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/mailbox.h>
> +#include <linux/pl320-ipc.h>
>  #include <linux/platform_device.h>
>  
>  #define HB_CPUFREQ_CHANGE_NOTE	0x80000001
> diff --git a/drivers/mailbox/pl320-ipc.c b/drivers/mailbox/pl320-ipc.c
> index d873cba..f3755e0 100644
> --- a/drivers/mailbox/pl320-ipc.c
> +++ b/drivers/mailbox/pl320-ipc.c
> @@ -26,7 +26,7 @@
>  #include <linux/device.h>
>  #include <linux/amba/bus.h>
>  
> -#include <linux/mailbox.h>
> +#include <linux/pl320-ipc.h>
>  
>  #define IPCMxSOURCE(m)		((m) * 0x40)
>  #define IPCMxDSET(m)		(((m) * 0x40) + 0x004)
> diff --git a/include/linux/mailbox.h b/include/linux/pl320-ipc.h
> similarity index 100%
> rename from include/linux/mailbox.h
> rename to include/linux/pl320-ipc.h
> 

Acked-by: Mark Langsdorf <mark.langsdorf@calxeda.com>

[PATCHv3 00/14] drivers: mailbox: framework creation

[Suman Anna]

Hi Andy,

On 04/26/2013 08:48 PM, Andy Green wrote:
> On 27/04/13 09:04, the mail apparently from Suman Anna included:
> 
> Hi Suman -
> 
>> Even though both the scenarios look very similar, I believe there are
>> some slight differences. All the devices belonging to a controller may
>> not be of the same type (meaning, intended towards the same remote or be
>> used interchangeably with one another). It is definitely possible if you
>> have a similar scenario to the DMA physical channels and your remote
>> rx interrupt can identify the device/channel to process. This would be
>> very much dependent on the architecture of a controller. The particular
>> example that I have in mind is s/w clients between the same set of
>> remote and host entities using the same device - the send part is anyway
>> arbitrated by the controller, and the same received message can be
>> delivered to the clients, with the clients making the decision whether
>> the packet belongs to them or not. I agree that all remote-ends will not
>> be able to cope up intermixed requests, but isn't this again a
>> controller architecture dependent?
> 
> Maybe it's helpful to describe our situation more concretely, because
> the problem is not coming from "the architecture of the [mailbox]
> controller".

Thanks for explaining the usecase. I do think that similar approaches
will become more common (TI AM335 has something similar as well - though
it is related to suspend). The right word should have been "controller
functional integration", I said it as s/w architecture or usage model.
In your case, it is clear that you need time-shared exclusive access,
whereas I am talking about simultaneous-shared usecases.

> 
> In the SoC we work on clock and subsystem power control registers, a
> serial bus, and some other assets are not directly accessible from
> Linux.  We must ask a coprocessor to operate these for us, using the
> mailbox.
> 
> So at any one time, the clock driver or voltagedomain driver for the SoC
> may want to own the mailbox and perform one or more operations over it
> synchronously, in some cases on the remote side involving transactions
> on a serial bus.  We don't want other transactions to be occurring while
> we wait for the serial bus to complete what the driver who started that
> asked for, for example.
> 
> We can cope with this by having an outer driver mediate access to the
> mailbox.  But then there are multiple sync primitives like completions
> and notifiers per operation, because your core already does this.
> 
> In short the FIFO + sync operations approach your core implements
> doesn't fit our use case.  That can be our problem, in which case we'll
> live with the outer mediation driver on top of the mailbox, or it can be
> a sign the fixed choice of FIFO + sync operations in your core did not
> quite hit the nail on the head to really model all the facets of legit
> mailbox usage.

I agree that the current code doesn't address this usage. The changes
(should have them ready in the next couple of days) I am working on
actually makes this conditional.

> 
> At least, this real scenario should be interesting to think about before
> rejecting ^^

No, I didn't reject anything, we are dealing with two contrasting
usecases dependent on the functional integration, and we have to find a
middle ground.

regards
Suman

[PATCHv3 00/14] drivers: mailbox: framework creation

[Suman Anna]

Hi Jassi,

On 04/26/2013 11:51 PM, Jassi Brar wrote:
> Hi Suman,
> 
>>> On 26 April 2013 03:59, Suman Anna <s-anna@ti.com> wrote:
>>>> On 04/25/2013 12:20 AM, Jassi Brar wrote:
> 
>>> I never said no-buffering and I never said buffering should be in
>>> controller drivers. In fact I don't remember ever objecting to how
>>> buffering is done in TI's framework.
>>> A controller could service only 1 request at a time so lets give it
>>> just 1 at a time. Let the API handle the complexity of buffering.
>>>
>>
>> Alright, guess this got lost in translation :). I interpreted based on
>> the fact that you wanted to get rid of the size field from the
>> mailbox_msg definition. Do you have a different mechanism in mind for
>> the buffering compared to the present one?
>>
> Sure, a very simple but efficient one. I had started on pseudo code
> implementation the day I first replied, but now I have real code with
> the PL320 controller and the Highbank client converted to the API. All
> that I say features in the new design. Polishing and documentation
> will take just a few hours more. You could see end to end what I have
> been talking about.
>>
>> OK, I didn't think of a no RTR interrupt-based controller. I would thing
>> that such a controller is very rudimentary. I wonder if there are any
>> controllers like this out there.
>>
> One of my controllers is like that :)

I hope it does have a status register atleast, and not the "neither
report nor sense RTR" type.

> 
>>>
>>> BTW, TI's RX mechanism too seems broken for common API. Receiving
>>> every few bytes via 'notify' mechanism is very inefficient. Imagine a
>>> platform with no shared memory between co-processors and the local
>>> wants to diagnose the remote by asking critical data at least KBs in
>>> size.
>>
>> No shared memory between co-processors and a relatively slow wire
>> transport is a bad architecture design to begin with.
>>
> IMHO it's only about private memory. Even if the controller transfers,
> say, 10bytes/interrupt there could always be a requirement to read
> some 1MB region of remote's private memory. And the same logic implies
> that our TX too should be as fast as possible - the remote might need
> its 1MB firmware over the link. So let us just try to serve all
> designs rather than evaluate them :)
> 
> 
>>>  So when API has nothing to do with received packet and the controller
>>> has to get rid of it asap so as to be able to receive the next, IMHO
>>> there should be short-circuit from controller to client via the API.
>>> No delay, no buffering of RX.
>>
>> The current TI design is based on the fact that we can get multiple
>> messages on a single interrupt due to the h/w fifo and the driver takes
>> care of the bottom-half. Leaving it to the client is putting a lot of
>> faith in the client and doesn't scale to multiple clients. The client
>> would have to perform mostly the same as the driver is doing - so this
>> goes back to the base discussion point that we have - which is the lack
>> of support for atomic_context receivers in the current code. I perceive
>> this as an attribute of the controller/mailbox device itself rather than
>> the client.
>>
> Sorry, I don't understand the concern about faith.
> If the controller h/w absolutely can not tell the remote(sender) of a
> received packet (as seems to be your case), its driver shouldn't even
> try to demux the received messages. The client driver must know which
> remotes could send it a message and how to discern them on the
> platform. Some 'server' RX client is needed here.

No demuxing, deliver the message to the different clients. It is a
protocol agreement between the clients on what the message means. Think
of this scenario akin to shared interrupts.

> If the controller could indeed map received packet onto remotes, then
> ideally the controller driver should declare one (RX only) channel for
> each such remote and demux packets onto them.
> In either case, 'notify' mechanism is not necessary.

The notify mechanism was the top-half on the interrupt handling. The
faith part is coming from the fact that you expect all the clients to do
the equivalent of the bottom-half (which would mean some duplication in
the different clients), the OMAP scenario is such that all the different
link interrupts (both rx & tx) are mapped onto a single physical
interrupt. I think this may not be applicable to your usecase, wherein
you probably expect a response back before proceeding.

> 
> 
>> I agree that all remote-ends will not
>> be able to cope up intermixed requests, but isn't this again a
>> controller architecture dependent?
>>
> I think it's more about remote's protocol implementation than
> controller's architecture.

Right, I meant functional integration.

> If tomorrow TI's remote firmware introduces a new set of critical
> commands that may arrive only in a particular sequence, you'll find
> yourself sharing a ride on our dinghy :)
> And Andy already explained where we come from.

This is almost always true when your remote is for offloading some h/w
operations.

regards
Suman

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

[Suman Anna]

On 04/27/2013 01:14 PM, jassisinghbrar at gmail.com wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
> 
> Convert the PL320 controller driver to work with the common
> mailbox API. Also convert the only user of PL320, highbank-cpufreq.c
> to work with thee API. Drop the obsoleted driver pl320-ipc.c

I think the conversion is fine based on your API, but you have
eliminated the stand-alone Rx interrupt code in the conversion. I
searched for if anybody is registering these rx atomic notifiers in 3.9,
and didn't find any. Is this expected to stay like this or is it some
future functionality not yet added, but getting removed in this patch.

> 
> Signed-off-by: Jassi Brar <jaswinder.singh@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?

regards
Suman

[PATCHv3 00/14] drivers: mailbox: framework creation

[Jassi Brar]

Hi

On 29 April 2013 21:30, Suman Anna <s-anna@ti.com> wrote:
> Hi Jassi,
>
> On 04/26/2013 11:51 PM, Jassi Brar wrote:

>>> OK, I didn't think of a no RTR interrupt-based controller. I would thing
>>> that such a controller is very rudimentary. I wonder if there are any
>>> controllers like this out there.
>>>
>> One of my controllers is like that :)
>
> I hope it does have a status register atleast, and not the "neither
> report nor sense RTR" type.
>
Actually the status is not set by the h/w, but the remote's firmware
implementation makes sure it sets a marker in the status register
after accepting data. So with some other firmware, we might not even
have the status read facility and the client will have to solely run
the TX ticker.

>> If the controller h/w absolutely can not tell the remote(sender) of a
>> received packet (as seems to be your case), its driver shouldn't even
>> try to demux the received messages. The client driver must know which
>> remotes could send it a message and how to discern them on the
>> platform. Some 'server' RX client is needed here.
>
> No demuxing, deliver the message to the different clients. It is a
> protocol agreement between the clients on what the message means. Think
> of this scenario akin to shared interrupts.
>
Please re-read. That's what I said - No demuxing by the controller driver :)

>
> The notify mechanism was the top-half on the interrupt handling. The
> faith part is coming from the fact that you expect all the clients to do
> the equivalent of the bottom-half (which would mean some duplication in
> the different clients), the OMAP scenario is such that all the different
> link interrupts (both rx & tx) are mapped onto a single physical
> interrupt. I think this may not be applicable to your usecase, wherein
> you probably expect a response back before proceeding.
>
Simply put - the RX by notifier method will fail should a client is
speed critical.
The api might provide it optionally, but direct handover should be the
default option.

Btw, I did put up an almost tested version of an API implementing most
of the features we agree upon
http://www.spinics.net/lists/kernel/msg1523873.html
http://www.spinics.net/lists/kernel/msg1523874.html

cheers.

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

[Jassi Brar]

On 29 April 2013 22:14, Suman Anna <s-anna@ti.com> wrote:
> On 04/27/2013 01:14 PM, jassisinghbrar at gmail.com wrote:
>> From: Jassi Brar <jaswinder.singh@linaro.org>
>>
>> Convert the PL320 controller driver to work with the common
>> mailbox API. Also convert the only user of PL320, highbank-cpufreq.c
>> to work with thee API. Drop the obsoleted driver pl320-ipc.c
>
> I think the conversion is fine based on your API, but you have
> eliminated the stand-alone Rx interrupt code in the conversion. I
> searched for if anybody is registering these rx atomic notifiers in 3.9,
> and didn't find any. Is this expected to stay like this or is it some
> future functionality not yet added, but getting removed in this patch.
>
Yeah, probably only some out-of-tree code needed that but that made my
life simpler :)

>>
>> Signed-off-by: Jassi Brar <jaswinder.singh@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.

cheers.

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

[Mark Langsdorf]

On 04/29/2013 11:57 AM, Jassi Brar wrote:
> On 29 April 2013 22:14, Suman Anna <s-anna@ti.com> wrote:
>> On 04/27/2013 01:14 PM, jassisinghbrar at gmail.com wrote:
>>> From: Jassi Brar <jaswinder.singh@linaro.org>
>>> Signed-off-by: Jassi Brar <jaswinder.singh@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.

--Mark Langsdorf
Calxeda, Inc.

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

[Jassi Brar]

On 29 April 2013 22:36, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote:
> On 04/29/2013 11:57 AM, Jassi Brar wrote:
>> On 29 April 2013 22:14, Suman Anna <s-anna@ti.com> wrote:
>>> On 04/27/2013 01:14 PM, jassisinghbrar at gmail.com wrote:
>>>> From: Jassi Brar <jaswinder.singh@linaro.org>
>>>> Signed-off-by: Jassi Brar <jaswinder.singh@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.

[RFC 2/3] mailbox: Introduce a new common API

[Suman Anna]

Hi Jassi,

On 04/27/2013 01:14 PM, jassisinghbrar at gmail.com wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
> 
> Introduce common framework for client/protocol drivers and
> controller drivers of Inter-Processor-Communication (IPC).
> 
> Client driver developers should have a look at
>  include/linux/mailbox_client.h to understand the part of
> the API exposed to client drivers.
> Similarly controller driver developers should have a look
> at include/linux/mailbox_controller.h

This implementation looks decent based on your design points. These
are the open/contention points that needs to be sorted out.

I think understanding the OMAP mailbox architecture also helps you,
since this series currently addressed PL320, but we will run into
some issues when adopting for OMAP as is. OMAP has a single mailbox IP,
each with multiple h/w FIFOs (fifo of length 4, and message size of a
u32). Each IP can cause one or more irq (usually 1) into the linux host.
It has status registers for number of messages in a FIFO (Rx), FIFO full
(Tx non-readiness/busy state), and interrupts for both Rx and Tx. There
are registers indicating the source of the interrupt, and these are per
FIFO. The Tx interrupt source is really for indicating Tx readiness or
that a fifo has open room for sending messages. This will keep on firing
as long as the FIFO is not-full, so we usually configure this only when
the FIFO is full and disable it the moment it is fired. It is re-enabled
when the h/w FIFO is full again, and we use s/w buffering in between.
The Rx interrupt is fired as long as there is a message in any FIFO
which has been configured to interrupt the host, so we have to empty all
the messages for that interrupt source.

Anyway, here is a summary of the open points that we have:
1. Atomic Callbacks:
The current code provides some sort of buffering on Tx, but imposes the
restriction that the clients do the buffering on Rx. This is main
concern for OMAP. We have multiple links on a single interrupt and
multiple messages per link on a interrupt source. Pushing this logic
into a client (remoteproc for us) is not right, that code does not
belong to the remoteproc driver, and neither is adding another layer in
between. It also kills the shared clients idea. The current
ipc_link_received_data seems open enough for me to use by calling it
from a work-queue, and have the OMAP controller driver take care of Rx
buffering (will test this next week). Restricting ipc_link_received_data
to atomic callbacks might have been enough for single transport
payloads, but we do have a FIFO. I could possibly make it work using
this API, but it brings me to my next point which is shared clients.

2. Support for Shared Clients AND Time-shared Clients:
As I said before, we need the framework to be flexible enough to support
shared clients. The shared clients may not be applicable for all h/w
controllers, where a client has to send a series of operations that
needs to be sent to the remote before anyone else can use it. This is a
functional integration aspect, and is dictated by the nature of the
remote. For the time-shared clients, the remote must have some implicit
message protocol where in the remote is able to identify the macro
usecase through a specific message. The framework should allow the case
of shared clients, with the clients doing their own message demuxing.

You can take a look at my patches on top of the existing mailbox code
that allows both shared clients and atomic callbacks, and multi-instance
support. It is not that hard to incorporate similar concepts into this
code as well. It is based on properties published by the
controller/link. [I haven't added the tx callbacks and poll method, that
would actually mean I will be doing a very similar state machine to what
you have, no point in me spending time on that, unless we are gonna
absorb the current mailbox series.]

https://github.com/sumananna/mailbox/commits/mailbox-multi-atomic-dt-doc

3. Buffering/Size: I think we need to take a re-look at the whole tx
buffering mechanism. You are using an array that stores just the
pointers, which means that there are underlying assumptions that the
clients are keeping their buffer ptrs intact until the duration of the
transfer is done. This means that one cannot use local payload
variables for messages in the calling functions. I feel this is
unnecessary burden on the clients.

Secondly, we may not need the logic around client knows_txdone and
tx_buffering together. I understand the need for buffering when you have
TX_POLL or TX_IRQ, but as far as TX_ACK is concerned, it is a client
protocol-level knowledge, and so the tx buffering logic can remain with
the client itself. This should simplify the code a little bit and get
rid of the ipc_client_txdone API.

Looking at the current use-cases, I think OMAP might be the only one
which needs the buffering. The API that Loic added suggests that he
either sends a message and expects a response, or just sends a message
if the transport is free (I am not sure if he is using the API that uses
buffering currently). PL320 is the same as the first scenario that Loic has.

The other point is regarding the size field, I am not convinced that we
can take this out, and leave it between the client and the controller
implementation. I think the mailbox core should at least perform a check
based on the max size published by a controller driver. Like in the
PL320 patch conversion, the for loop for writing the data - how does it
know that the provided pointer is atleast 7 ints, and not smaller than
that?

Rest of the comments are in the code...

> 
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> new file mode 100644
> index 0000000..c5ec93e
> --- /dev/null
> +++ b/drivers/mailbox/mailbox.c
> +
> +/*
> + * Called by a client to "put data on the h/w channel" so that if
> + * everything else is fine we don't need to do anything more locally
> + * for the remote to receive the data intact.
> + * In reality, the remote may receive it intact, corrupted or not at all.
> + * This could be called from atomic context as it simply
> + * queues the data and returns a token (request_token_t)
> + * against the request.
> + * The client is later notified of successful transmission of
> + * data over the channel via the 'txcb'. The client could in
> + * turn queue more messages from txcb.
> + */
> +request_token_t ipc_send_message(void *channel, void *data)
> +{
> +	struct ipc_chan *chan = (struct ipc_chan *)channel;
> +	request_token_t t;
> +
> +	if (!chan)
> +		return 0;
> +
> +	t = _add_to_rbuf(chan, data);
> +	if (!t)
> +		printk("Try increasing MBOX_TX_QUEUE_LEN\n");
> +
> +	_msg_submit(chan);
> +
> +	if (chan->txdone_method	== TXDONE_BY_POLL)
> +		poll_txdone((unsigned long)chan->timer);
> +
> +	if (chan->tx_block && chan->active_token) {

Assigning the tx_block at channel request time might not be flexible
enough. We should not impose that the client will request and release
channels with different modes if both scenarios are needed.

> +		int ret;
> +		init_completion(&chan->tx_complete);
> +		ret = wait_for_completion_timeout(&chan->tx_complete,
> +			chan->tx_tout);
> +		if (ret == 0) {
> +			t = 0;
> +			tx_tick(chan, XFER_ERR);
> +		}
> +	}
> +
> +	return t;
> +}
> +EXPORT_SYMBOL(ipc_send_message);

Do we need to add an API similar to mailbox_msg_send_receive_no_irq, or
are you leaving it to be client's responsibility?

> +
> +/*
> + * A client driver asks for exclusive use of a channel/mailbox.
> + * If assigned, the channel has to be 'freed' before it could
> + * be assigned to some other client.
> + * After assignment, any packet received on this channel will be
> + * handed over to the client via the 'rxcb' callback.
> + * The 'txcb' callback is used to notify client upon sending the
> + * packet over the channel, which may or may not have been yet
> + * read by the remote processor.
> + */
> +void *ipc_request_channel(struct ipc_client *cl)
> +{
> +	struct ipc_chan *chan;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	mutex_lock(&chpool_mutex);
> +
> +	list_for_each_entry(chan, &ipc_channels, node)
I had a different design in mind here, maintain the list of controllers
globally instead of the channels, that way your search can be a bit
quicker.

> +		if(!chan->assigned
> +				&& !strcmp(cl->chan_name, chan->chan_name)) {
> +			spin_lock_irqsave(&chan->lock, flags);
> +			chan->msg_free = 0;
> +			chan->msg_count = 0;
> +			chan->active_token = 0;
> +			chan->rxcb = cl->rxcb;
> +			chan->txcb = cl->txcb;
> +			chan->assigned = true;
> +			chan->tx_block = cl->tx_block;
> +			if (!cl->tx_tout)
> +				chan->tx_tout = ~0;
> +			else
> +				chan->tx_tout = msecs_to_jiffies(cl->tx_tout);
> +			if (chan->txdone_method	== TXDONE_BY_POLL
> +					&& cl->knows_txdone)
> +				chan->txdone_method |= TXDONE_BY_ACK;
> +			spin_unlock_irqrestore(&chan->lock, flags);
> +			ret = 1;
> +			break;
> +		}
> +
> +	mutex_unlock(&chpool_mutex);
> +
> +	if (!ret) {
> +		printk("Unable to assign mailbox(%s)\n", cl->chan_name);
> +		return NULL;
> +	}
> +
> +	ret = chan->link_ops->startup(chan->link, cl->cntlr_data);
> +	if (ret) {
> +		printk("Unable to startup the link\n");
> +		ipc_free_channel((void *)chan);
> +		return NULL;
> +	}
> +
> +	return (void *)chan;
> +}
> +EXPORT_SYMBOL(ipc_request_channel);
> +
> +/*
> + * Call for IPC controller drivers to register a controller, adding
> + * its channels/mailboxes to the global pool.
> + */
> +int ipc_links_register(struct ipc_controller *ipc_con)
> +{
> +	struct tx_poll_timer *timer = NULL;
> +	struct ipc_chan *channel;
> +	int i, num_links, txdone;
> +
> +	/* Are you f***ing with us, sir? */
> +	if (!ipc_con || !ipc_con->ops)
> +		return -EINVAL;
> +
> +	for (i = 0; ipc_con->links[i]; i++)
> +		;
> +	if (!i)
> +		return -EINVAL;
> +	num_links = i;
> +
> +	if (ipc_con->txdone_irq)
> +		txdone = TXDONE_BY_IRQ;
> +	else if (ipc_con->txdone_poll)
> +		txdone = TXDONE_BY_POLL;
> +	else /* It has to be at least ACK */
> +		txdone = TXDONE_BY_ACK;
> +
> +	if (txdone == TXDONE_BY_POLL) {
> +		timer = kzalloc(sizeof(struct tx_poll_timer), GFP_KERNEL);
> +		timer->period = ipc_con->txpoll_period;
> +		timer->poll.function = &poll_txdone;
> +		timer->poll.data = (unsigned long)timer;
> +		init_timer(&timer->poll);
> +	}
> +
> +	channel = kzalloc(sizeof(struct ipc_chan) * num_links, GFP_KERNEL);

minor, but you might be aware of this already, failure check needed

> +
> +	for (i = 0; i < num_links; i++) {
> +		channel[i].timer = timer;
> +		channel[i].assigned = false;
> +		channel[i].txdone_method = txdone;
> +		channel[i].link_ops = ipc_con->ops;
> +		channel[i].link = ipc_con->links[i];
> +		channel[i].link->api_priv = &channel[i];
> +		snprintf(channel[i].chan_name, 32, "%s:%s",
> +			ipc_con->controller_name,
> +			ipc_con->links[i]->link_name);
> +		spin_lock_init(&channel[i].lock);
> +		BLOCKING_INIT_NOTIFIER_HEAD(&channel[i].avail);
> +		mutex_lock(&chpool_mutex);
> +		list_add_tail(&channel[i].node, &ipc_channels);
> +		mutex_unlock(&chpool_mutex);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ipc_links_register);
> +
> +static int __init ipc_mbox_init(void)
> +{
> +	INIT_LIST_HEAD(&ipc_channels);
> +	return 0;
> +}
> +subsys_initcall(ipc_mbox_init);

The ipc_mbox_init can be replaced by static list init of ipc_channels,
nothing much to be gained by defining this function. We expect the
mailbox.c to be built anyways whenever the mailbox framework is selected.

> diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
> new file mode 100644
> index 0000000..33545f6
> --- /dev/null
> +++ b/include/linux/mailbox_client.h
> +/**
> + * struct ipc_client - User of a mailbox
> + * @chan_name: the "controller:channel" this client wants
> + * @rxcb: atomic callback to provide client the data received
> + * @txcb: atomic callback to tell client of data transmission
> + * @tx_block: if the ipc_send_message should block until data is transmitted
> + * @tx_tout: Max block period in ms before TX is assumed failure
> + * @knows_txdone: if the client could run the TX state machine. Usually if
> + *    the client receives some ACK packet for transmission. Unused if the
> + *    controller already has TX_Done/RTR IRQ.
> + * @cntlr_data: Optional controller specific parameters during channel request
> + */
> +struct ipc_client {
> +	char *chan_name;
> +	void (*rxcb)(void *data);
> +	void (*txcb)(request_token_t t, enum xfer_result r);
> +	bool tx_block;
> +	unsigned long tx_tout;
> +	bool knows_txdone;
> +	void *cntlr_data;

What is the current use-case for exposing cntrl_data through ipc_client?
I think this should be avoided and leave the controller configuration to
the controller driver implementation. This will be a problem for
multiple link scenarios.

> diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
> new file mode 100644
> index 0000000..defcfb3
> --- /dev/null
> +++ b/include/linux/mailbox_controller.h
> +
> +/**
> + * struct ipc_link - s/w representation of a communication link
> + * @link_name: Literal name assigned to the link. Physically
> + *    identical channels may have the same name.
> + * @api_priv: hook for the API to map its private data on the link
> + *    Controller driver must not touch it.
> + */
> +struct ipc_link {
> +	char link_name[16];
> +	void *api_priv;
> +};

this definitely needs a link-private void *ptr. PL320 doesn't have
multiple links in the controller, but this is a problem for OMAP & DBX500.

> +struct ipc_link_ops {
> +	int (*send_data)(struct ipc_link *link, void *data);
> +	int (*startup)(struct ipc_link *link, void *params);
> +	void (*shutdown)(struct ipc_link *link);
> +	bool (*last_tx_done)(struct ipc_link *link);
minor comment, maybe rename this to something that indicates link
busyness or readiness

> +};
> +
> +/**
> + * struct ipc_controller - Controller of a class of communication links
> + * @controller_name: Literal name of the controller.
> + * @ops: Operators that work on each communication link
> + * @links: Null terminated array of links.
> + * @txdone_irq: Indicates if the controller can report to API when the
> + *    last transmitted data was read by the remote. Eg, if it has some
> + *    TX ACK irq.
> + * @txdone_poll: If the controller can read but not report the TX done.
> + *    Eg, is some register shows the TX status but no interrupt rises.
> + *    Ignored if 'txdone_irq' is set.
> + * @txpoll_period: If 'txdone_poll' is in effect, the API polls for
> + *    last TX's status after these many millisecs
> + */
> +struct ipc_controller {
> +	char controller_name[16];
i think this can be avoided and use the underlying dev-name directly
based on the controller device. Adding a struct device pointer would
automatically allow you to use the name from the probe.

> +	struct ipc_link_ops *ops;
I think this should be a property of the individual link for
flexibility. Right now, it probably doesn't make a difference (as seen
from the current mailbox code), but the moment we have one link behaving
differently this will make the ops implementation a bit messy.

I think we also need a controller-specific ops, to put common stuff
between multiple links within the controller.

> +	struct ipc_link **links;
> +	bool txdone_irq;
> +	bool txdone_poll;
> +	unsigned txpoll_period;
> +};
> +

[RFC 2/3] mailbox: Introduce a new common API

[Jassi Brar]

Hi Suman,

On 4 May 2013 07:50, Suman Anna <s-anna@ti.com> wrote:
> Hi Jassi,
>
> On 04/27/2013 01:14 PM, jassisinghbrar at gmail.com wrote:
>> From: Jassi Brar <jaswinder.singh@linaro.org>
>>
>> Introduce common framework for client/protocol drivers and
>> controller drivers of Inter-Processor-Communication (IPC).
>>
>> Client driver developers should have a look at
>>  include/linux/mailbox_client.h to understand the part of
>> the API exposed to client drivers.
>> Similarly controller driver developers should have a look
>> at include/linux/mailbox_controller.h
>
> This implementation looks decent based on your design points. These
> are the open/contention points that needs to be sorted out.
>
> I think understanding the OMAP mailbox architecture also helps you,
> since this series currently addressed PL320, but we will run into
> some issues when adopting for OMAP as is. OMAP has a single mailbox IP,
> each with multiple h/w FIFOs (fifo of length 4, and message size of a
> u32). Each IP can cause one or more irq (usually 1) into the linux host.
> It has status registers for number of messages in a FIFO (Rx), FIFO full
> (Tx non-readiness/busy state), and interrupts for both Rx and Tx. There
> are registers indicating the source of the interrupt, and these are per
> FIFO. The Tx interrupt source is really for indicating Tx readiness or
> that a fifo has open room for sending messages. This will keep on firing
> as long as the FIFO is not-full, so we usually configure this only when
> the FIFO is full and disable it the moment it is fired. It is re-enabled
> when the h/w FIFO is full again, and we use s/w buffering in between.
> The Rx interrupt is fired as long as there is a message in any FIFO
> which has been configured to interrupt the host, so we have to empty all
> the messages for that interrupt source.
>
Yeah, thanks for explanation. I've worked on OMAP for almost 2 yrs
now, though not mailbox. I did have a look at the OMAP4430 trm.
 I understand OMAP's MBox doesn't really have TX-Done/RTR interrupt,
it only has "Tx Buffer Not Full" interrupt which serves the purpose
only for the window when there are at least 4 TX messages pending. Now
the driver could switch between Polling and IRQ at runtime depending
upon the buffer filled extent, OR simply work in polling mode all the
time. IMHO the controller driver should opt for the latter.  Though it
might be interesting to profile out of total transfers during a
period, how many are actually queued till depth of 4.


> Anyway, here is a summary of the open points that we have:
> 1. Atomic Callbacks:
> The current code provides some sort of buffering on Tx, but imposes the
> restriction that the clients do the buffering on Rx. This is main
> concern for OMAP.

I am afraid a common API can't do without buffering TX and it can't do
by buffering RX.

 The client(s) can always generate TX requests at a rate greater than
the API could transmit on the physical link. So as much as we dislike
it, we have to buffer TX requests, otherwise N clients would.
 OTOH Buffering received packets in the API doesn't help anybody, it
only incurs unavoidable latencies on clients. Only clients, that need
to take non-atomic actions upon RX, would need to buffer RX. Other
clients should not suffer.

IMHO if clients on OMAP need to buffer RX, let us keep it OMAP
specific. If number of such platforms rise in future we could move
that as an _optional_ helper API on top, that does RX buffering on
behalf of clients ?


> 2. Support for Shared Clients AND Time-shared Clients:
> As I said before, we need the framework to be flexible enough to support
> shared clients. The shared clients may not be applicable for all h/w
> controllers, where a client has to send a series of operations that
> needs to be sent to the remote before anyone else can use it. This is a
> functional integration aspect, and is dictated by the nature of the
> remote. For the time-shared clients, the remote must have some implicit
> message protocol where in the remote is able to identify the macro
> usecase through a specific message. The framework should allow the case
> of shared clients, with the clients doing their own message demuxing.
>
 If the API provides shared ownership of a mailbox, it won't work for
clients that need exclusive ownership (like 'server' side
implementation of a protocol).
 OTOH if the API provides exclusive ownership, it is still possible to
emulate shared ownership by simply publishing the mailbox handle (void
*chan) globally. It works for all.

>
> 3. Buffering/Size: I think we need to take a re-look at the whole tx
> buffering mechanism. You are using an array that stores just the
> pointers, which means that there are underlying assumptions that the
> clients are keeping their buffer ptrs intact until the duration of the
> transfer is done. This means that one cannot use local payload
> variables for messages in the calling functions. I feel this is
> unnecessary burden on the clients.
>
Most of the clients won't queue more than 1 request at a time. And
then, isn't it only natural that clients don't mess with requests
after submitting them ?  I see mailbox clients working quite like I2C
clients.

> Secondly, we may not need the logic around client knows_txdone and
> tx_buffering together. I understand the need for buffering when you have
> TX_POLL or TX_IRQ, but as far as TX_ACK is concerned, it is a client
> protocol-level knowledge, and so the tx buffering logic can remain with
> the client itself. This should simplify the code a little bit and get
> rid of the ipc_client_txdone API.
>
Yeah I had it that way originally. But then I realized we could be
running an 'ACK Packet' protocol over a controller that supports only
POLL. In that case the controller's poll doesn't override client's
knows_txdone because we want the client to cut-short the next poll
delay as soon as it gets an ACK packet.

> Looking at the current use-cases, I think OMAP might be the only one
> which needs the buffering. The API that Loic added suggests that he
> either sends a message and expects a response, or just sends a message
> if the transport is free (I am not sure if he is using the API that uses
> buffering currently). PL320 is the same as the first scenario that Loic has.
>
Yeah I too expect TX buffering to be very rare.

> The other point is regarding the size field, I am not convinced that we
> can take this out, and leave it between the client and the controller
> implementation. I think the mailbox core should at least perform a check
> based on the max size published by a controller driver. Like in the
> PL320 patch conversion, the for loop for writing the data - how does it
> know that the provided pointer is atleast 7 ints, and not smaller than
> that?
>
First of all, PL320 is a highly configurable and programmable IP.
pl320.c is very specific to Highbank's usage, it is not usable as such
on other platforms. There are many things hardcoded and tacitly
assumed in the driver. I just changed the API, didn't add anything
new.

Secondly, I believe we acknowledged the fact that a client can't do
without controller specific knowledge. So in proper implementation the
controller's header would look like

struct foobar_con_mssg {
   int datalen; /* size of received packet in bytes */
   u8 *buf;  /* buffer that holds the packet data */
    .....
};

So instead of telling the API the 'datalen' and the remaining
structure of each message, we simply pass "struct foobar_con_mssg *"
as a void* to the API.


>> +
>> +/*
>> + * Called by a client to "put data on the h/w channel" so that if
>> + * everything else is fine we don't need to do anything more locally
>> + * for the remote to receive the data intact.
>> + * In reality, the remote may receive it intact, corrupted or not at all.
>> + * This could be called from atomic context as it simply
>> + * queues the data and returns a token (request_token_t)
>> + * against the request.
>> + * The client is later notified of successful transmission of
>> + * data over the channel via the 'txcb'. The client could in
>> + * turn queue more messages from txcb.
>> + */
>> +request_token_t ipc_send_message(void *channel, void *data)
>> +{
>> +     struct ipc_chan *chan = (struct ipc_chan *)channel;
>> +     request_token_t t;
>> +
>> +     if (!chan)
>> +             return 0;
>> +
>> +     t = _add_to_rbuf(chan, data);
>> +     if (!t)
>> +             printk("Try increasing MBOX_TX_QUEUE_LEN\n");
>> +
>> +     _msg_submit(chan);
>> +
>> +     if (chan->txdone_method == TXDONE_BY_POLL)
>> +             poll_txdone((unsigned long)chan->timer);
>> +
>> +     if (chan->tx_block && chan->active_token) {
>
> Assigning the tx_block at channel request time might not be flexible
> enough. We should not impose that the client will request and release
> channels with different modes if both scenarios are needed.
>
If there is one blocking request, no more could arrive (blocking or
non-blocking). The increased code complexity doesn't justify the
rarely used feature. I don't see many use-cases when the client can't
release-request the channel. It's made like opening a file - blocking
vs non-blocking is the mode you open the resource.

>> +             int ret;
>> +             init_completion(&chan->tx_complete);
>> +             ret = wait_for_completion_timeout(&chan->tx_complete,
>> +                     chan->tx_tout);
>> +             if (ret == 0) {
>> +                     t = 0;
>> +                     tx_tick(chan, XFER_ERR);
>> +             }
>> +     }
>> +
>> +     return t;
>> +}
>> +EXPORT_SYMBOL(ipc_send_message);
>
> Do we need to add an API similar to mailbox_msg_send_receive_no_irq, or
> are you leaving it to be client's responsibility?
>
Yes the API provides enough ways for a client to do that.

>> +
>> +/*
>> + * A client driver asks for exclusive use of a channel/mailbox.
>> + * If assigned, the channel has to be 'freed' before it could
>> + * be assigned to some other client.
>> + * After assignment, any packet received on this channel will be
>> + * handed over to the client via the 'rxcb' callback.
>> + * The 'txcb' callback is used to notify client upon sending the
>> + * packet over the channel, which may or may not have been yet
>> + * read by the remote processor.
>> + */
>> +void *ipc_request_channel(struct ipc_client *cl)
>> +{
>> +     struct ipc_chan *chan;
>> +     unsigned long flags;
>> +     int ret = 0;
>> +
>> +     mutex_lock(&chpool_mutex);
>> +
>> +     list_for_each_entry(chan, &ipc_channels, node)
> I had a different design in mind here, maintain the list of controllers
> globally instead of the channels, that way your search can be a bit
> quicker.
>
Yeah, I changed that to it last minute :)
The API has no real use of controller nodes, it works solely on a
global pool of mailboxes. I tried to reflect that in code.


>> +
>> +     if (txdone == TXDONE_BY_POLL) {
>> +             timer = kzalloc(sizeof(struct tx_poll_timer), GFP_KERNEL);
>> +             timer->period = ipc_con->txpoll_period;
>> +             timer->poll.function = &poll_txdone;
>> +             timer->poll.data = (unsigned long)timer;
>> +             init_timer(&timer->poll);
>> +     }
>> +
>> +     channel = kzalloc(sizeof(struct ipc_chan) * num_links, GFP_KERNEL);
>
> minor, but you might be aware of this already, failure check needed
>
No, I overlooked. Thanks.


>> +
>> +static int __init ipc_mbox_init(void)
>> +{
>> +     INIT_LIST_HEAD(&ipc_channels);
>> +     return 0;
>> +}
>> +subsys_initcall(ipc_mbox_init);
>
> The ipc_mbox_init can be replaced by static list init of ipc_channels,
> nothing much to be gained by defining this function. We expect the
> mailbox.c to be built anyways whenever the mailbox framework is selected.
>
Yes, you are right. Thanks.

>> + * @cntlr_data: Optional controller specific parameters during channel request
>> + */
>> +struct ipc_client {
>> +     char *chan_name;
>> +     void (*rxcb)(void *data);
>> +     void (*txcb)(request_token_t t, enum xfer_result r);
>> +     bool tx_block;
>> +     unsigned long tx_tout;
>> +     bool knows_txdone;
>> +     void *cntlr_data;
>
> What is the current use-case for exposing cntrl_data through ipc_client?
> I think this should be avoided and leave the controller configuration to
> the controller driver implementation. This will be a problem for
> multiple link scenarios.
>
There are some highly programmable controllers that, if the driver is
proper, could change behavior runtime. For ex, if the controller
supports, a client might want to broadcast messages to multiple
remotes or listen for messages from more than one remote source or
both. The client could, say, specify bitmasks for such source/sink
remotes via cntlr_data while requesting the mailbox.  PL320 could work
in such mode if rest all fits.


>> +struct ipc_link {
>> +     char link_name[16];
>> +     void *api_priv;
>> +};
>
> this definitely needs a link-private void *ptr. PL320 doesn't have
> multiple links in the controller, but this is a problem for OMAP & DBX500.
>
The API keeps the ipc_link provided by the controller driver, which
could have it embedded in its private representation of the h/w links.
 For ex, my controller has 6 links, and its structure contains 6
element array of 'struct myhw_link' which embeds an ipc_link. And I
use container_of().


>> +struct ipc_link_ops {
>> +     int (*send_data)(struct ipc_link *link, void *data);
>> +     int (*startup)(struct ipc_link *link, void *params);
>> +     void (*shutdown)(struct ipc_link *link);
>> +     bool (*last_tx_done)(struct ipc_link *link);
> minor comment, maybe rename this to something that indicates link
> busyness or readiness
>
The API gives the controller only one TX at a time, so I chose the
name. What do you suggest?


>> +};
>> +
>> +/**
>> + * struct ipc_controller - Controller of a class of communication links
>> + * @controller_name: Literal name of the controller.
>> + * @ops: Operators that work on each communication link
>> + * @links: Null terminated array of links.
>> + * @txdone_irq: Indicates if the controller can report to API when the
>> + *    last transmitted data was read by the remote. Eg, if it has some
>> + *    TX ACK irq.
>> + * @txdone_poll: If the controller can read but not report the TX done.
>> + *    Eg, is some register shows the TX status but no interrupt rises.
>> + *    Ignored if 'txdone_irq' is set.
>> + * @txpoll_period: If 'txdone_poll' is in effect, the API polls for
>> + *    last TX's status after these many millisecs
>> + */
>> +struct ipc_controller {
>> +     char controller_name[16];
> i think this can be avoided and use the underlying dev-name directly
> based on the controller device. Adding a struct device pointer would
> automatically allow you to use the name from the probe.
>
Ah ha... yes. Thanks.

>> +     struct ipc_link_ops *ops;
> I think this should be a property of the individual link for
> flexibility. Right now, it probably doesn't make a difference (as seen
> from the current mailbox code), but the moment we have one link behaving
> differently this will make the ops implementation a bit messy.
>
> I think we also need a controller-specific ops, to put common stuff
> between multiple links within the controller.
>
A controller is assumed to be a bunch of identical links/mailboxes.
However the cntlr_data could still help the client specify mode in
which it wants a mailbox behave.

Thanks and Regards.
-Jassi

[RFC 2/3] mailbox: Introduce a new common API

[Suman Anna]

Hi Jassi,

On 05/04/2013 02:08 PM, Jassi Brar wrote:
> Hi Suman,
> 
>> Anyway, here is a summary of the open points that we have:
>> 1. Atomic Callbacks:
>> The current code provides some sort of buffering on Tx, but imposes the
>> restriction that the clients do the buffering on Rx. This is main
>> concern for OMAP.
> 
> I am afraid a common API can't do without buffering TX and it can't do
> by buffering RX.

Maybe both these are properties of the controller then.

> 
>  The client(s) can always generate TX requests at a rate greater than
> the API could transmit on the physical link. So as much as we dislike
> it, we have to buffer TX requests, otherwise N clients would.

The current code doesn't support N clients today anyway, and if they are
blocking mode on top of it, it would never need Tx buffering.

>
> OTOH Buffering received packets in the API doesn't help anybody, it
> only incurs unavoidable latencies on clients. Only clients, that need
> to take non-atomic actions upon RX, would need to buffer RX. Other
> clients should not suffer.

As I have explained for OMAP, it is about performing the bottom-half of
the Rx interrupt in the OMAP mailbox controller driver. The clients just
process the message (independent of inherent knowledge that they all
have to behave in a certain way to perform the bottom-half). A single
transport payload used for both Rx and Tx would make the state machine
very simple.

> 
> IMHO if clients on OMAP need to buffer RX, let us keep it OMAP
> specific. If number of such platforms rise in future we could move
> that as an _optional_ helper API on top, that does RX buffering on
> behalf of clients ?

Yeah, that's my idea as well to put this in the OMAP mailbox controller
driver implementation. The IPC stacks on OMAP are quite established
(both in remoteproc and DSP/Bridge - different generations), so I cannot
afford to break the functionality of either of those. This already means
I have to export some API from my controller driver and there is no
other way or now.

> 
>> 2. Support for Shared Clients AND Time-shared Clients:
>> As I said before, we need the framework to be flexible enough to support
>> shared clients. The shared clients may not be applicable for all h/w
>> controllers, where a client has to send a series of operations that
>> needs to be sent to the remote before anyone else can use it. This is a
>> functional integration aspect, and is dictated by the nature of the
>> remote. For the time-shared clients, the remote must have some implicit
>> message protocol where in the remote is able to identify the macro
>> usecase through a specific message. The framework should allow the case
>> of shared clients, with the clients doing their own message demuxing.
>>
>  If the API provides shared ownership of a mailbox, it won't work for
> clients that need exclusive ownership (like 'server' side
> implementation of a protocol).
>  OTOH if the API provides exclusive ownership, it is still possible to
> emulate shared ownership by simply publishing the mailbox handle (void
> *chan) globally. It works for all.

I am not saying provide this always. Have this dictated by the
controller or mailbox (look at my branch). The global publication works
well functionality-wise for Tx, but not so much for Rx. In anycase,
global publication will have its own set of problems - mostly you are
implying another layer of implementation that provides this sharing
capability (since they have to be outside of any single client).

> 
>>
>> 3. Buffering/Size: I think we need to take a re-look at the whole tx
>> buffering mechanism. You are using an array that stores just the
>> pointers, which means that there are underlying assumptions that the
>> clients are keeping their buffer ptrs intact until the duration of the
>> transfer is done. This means that one cannot use local payload
>> variables for messages in the calling functions. I feel this is
>> unnecessary burden on the clients.
>>
> Most of the clients won't queue more than 1 request at a time. And
> then, isn't it only natural that clients don't mess with requests
> after submitting them ?  I see mailbox clients working quite like I2C
> clients.

The OMAP use-cases do this today, so not a good assumption to make. The
OMAP IPC is built upon simple transmitting and processing received
messages separately, we will not be using blocking mode or tx callbacks.
I really don't need the complexity of having to introduce tx callbacks
just because I need to know if my pointer can be freed, as I said - it
eliminates the simplicity of using local variables unless the send API
is blocking. void * works for OMAP since my payloads are only 4 bytes by
themselves (this is a discussion that came up during the earlier series
as well), but this is not flexible enough on the clients-side generally.

> 
>> Secondly, we may not need the logic around client knows_txdone and
>> tx_buffering together. I understand the need for buffering when you have
>> TX_POLL or TX_IRQ, but as far as TX_ACK is concerned, it is a client
>> protocol-level knowledge, and so the tx buffering logic can remain with
>> the client itself. This should simplify the code a little bit and get
>> rid of the ipc_client_txdone API.
>>
> Yeah I had it that way originally. But then I realized we could be
> running an 'ACK Packet' protocol over a controller that supports only
> POLL. In that case the controller's poll doesn't override client's
> knows_txdone because we want the client to cut-short the next poll
> delay as soon as it gets an ACK packet.

I think the same can be achieved from the Rx path upon an interrupt in
the specific controller, without having to introduce the knows_txdone.
It is knowledge intrinsic to a controller/mailbox. I really hope that
there are no controllers where you have to poll for Rx too :)

> 
>> Looking at the current use-cases, I think OMAP might be the only one
>> which needs the buffering. The API that Loic added suggests that he
>> either sends a message and expects a response, or just sends a message
>> if the transport is free (I am not sure if he is using the API that uses
>> buffering currently). PL320 is the same as the first scenario that Loic has.
>>
> Yeah I too expect TX buffering to be very rare.

If so, can we eliminate this for the first pass, and leave it to the
controllers for now or dictate this based on a tx_buffering property? As
I see it, we do not yet have an agreement on the Tx buffering semantics.

> 
>> The other point is regarding the size field, I am not convinced that we
>> can take this out, and leave it between the client and the controller
>> implementation. I think the mailbox core should at least perform a check
>> based on the max size published by a controller driver. Like in the
>> PL320 patch conversion, the for loop for writing the data - how does it
>> know that the provided pointer is atleast 7 ints, and not smaller than
>> that?
>>
> First of all, PL320 is a highly configurable and programmable IP.
> pl320.c is very specific to Highbank's usage, it is not usable as such
> on other platforms. There are many things hardcoded and tacitly
> assumed in the driver. I just changed the API, didn't add anything
> new.

OK, should we be renaming this file accordingly then until another
common user comes along? Also, an appropriate 'depends on' in the Kconfig.

> 
> Secondly, I believe we acknowledged the fact that a client can't do
> without controller specific knowledge. So in proper implementation the
> controller's header would look like
> 
> struct foobar_con_mssg {
>    int datalen; /* size of received packet in bytes */
>    u8 *buf;  /* buffer that holds the packet data */
>     .....
> };
> 
> So instead of telling the API the 'datalen' and the remaining
> structure of each message, we simply pass "struct foobar_con_mssg *"
> as a void* to the API.

This has to be documented very well. And yes, I did acknowledge the
inter-dependencies portion for understanding the packet structure (there
is no way if you have a packet of 6 ints and you expect specific ints
for specific fields). The size and void * are generic enough that can be
present in the core to perform some error checking. The API would be
akin to memcpy then. The other motivation is based on the buffering
scheme (same comment as above on Tx buffering), which we haven't agreed
upon completely.

> 
> 
>>> +
>>> +/*
>>> + * Called by a client to "put data on the h/w channel" so that if
>>> + * everything else is fine we don't need to do anything more locally
>>> + * for the remote to receive the data intact.
>>> + * In reality, the remote may receive it intact, corrupted or not at all.
>>> + * This could be called from atomic context as it simply
>>> + * queues the data and returns a token (request_token_t)
>>> + * against the request.
>>> + * The client is later notified of successful transmission of
>>> + * data over the channel via the 'txcb'. The client could in
>>> + * turn queue more messages from txcb.
>>> + */
>>> +request_token_t ipc_send_message(void *channel, void *data)
>>> +{
>>> +     struct ipc_chan *chan = (struct ipc_chan *)channel;
>>> +     request_token_t t;
>>> +
>>> +     if (!chan)
>>> +             return 0;
>>> +
>>> +     t = _add_to_rbuf(chan, data);
>>> +     if (!t)
>>> +             printk("Try increasing MBOX_TX_QUEUE_LEN\n");
>>> +
>>> +     _msg_submit(chan);
>>> +
>>> +     if (chan->txdone_method == TXDONE_BY_POLL)
>>> +             poll_txdone((unsigned long)chan->timer);
>>> +
>>> +     if (chan->tx_block && chan->active_token) {
>>
>> Assigning the tx_block at channel request time might not be flexible
>> enough. We should not impose that the client will request and release
>> channels with different modes if both scenarios are needed.
>>
> If there is one blocking request, no more could arrive (blocking or
> non-blocking).

Well, you are assuming that there is only one thread from within the
client driver who is gonna use the send API. There is nothing in the
code that stops from say two threads/contexts queueing in the message
from the same client driver. I am ok from my side for this to be sorted
out later, since I won't be using the blocking mode, but I do have two
threads (in DSPBridge driver) that will do a send.

 The increased code complexity doesn't justify the
> rarely used feature. I don't see many use-cases when the client can't
> release-request the channel. It's made like opening a file - blocking
> vs non-blocking is the mode you open the resource.
> 
>>> +             int ret;
>>> +             init_completion(&chan->tx_complete);
>>> +             ret = wait_for_completion_timeout(&chan->tx_complete,
>>> +                     chan->tx_tout);
>>> +             if (ret == 0) {
>>> +                     t = 0;
>>> +                     tx_tick(chan, XFER_ERR);
>>> +             }
>>> +     }
>>> +
>>> +     return t;
>>> +}
>>> +EXPORT_SYMBOL(ipc_send_message);
>>
>> Do we need to add an API similar to mailbox_msg_send_receive_no_irq, or
>> are you leaving it to be client's responsibility?
>>
> Yes the API provides enough ways for a client to do that.

I will let Loic comment on this, based on his usage/complexity in the
client. I think you got lucky with PL320 implementation since it fills
in the buffer on a Tx done, so you have folded that functionality in
send_data itself.

> 
>>> +void *ipc_request_channel(struct ipc_client *cl)
>>> +{
>>> +     struct ipc_chan *chan;
>>> +     unsigned long flags;
>>> +     int ret = 0;
>>> +
>>> +     mutex_lock(&chpool_mutex);
>>> +
>>> +     list_for_each_entry(chan, &ipc_channels, node)
>> I had a different design in mind here, maintain the list of controllers
>> globally instead of the channels, that way your search can be a bit
>> quicker.
>>
> Yeah, I changed that to it last minute :)
> The API has no real use of controller nodes, it works solely on a
> global pool of mailboxes. I tried to reflect that in code.

Can we go back to using controller nodes? I understand that the API
doesn't use much, but the list of controller nodes is gonna be static
anyway in the file. It just improves the search algorithm when looking
for a mailbox. We have a new device coming up (u-boot patches already
submitted) wherein we have 13 controller instances, each with about 4 to
6 links, a linear search would be really painful.

> 
> 
>>> + * @cntlr_data: Optional controller specific parameters during channel request
>>> + */
>>> +struct ipc_client {
>>> +     char *chan_name;
>>> +     void (*rxcb)(void *data);
>>> +     void (*txcb)(request_token_t t, enum xfer_result r);
>>> +     bool tx_block;
>>> +     unsigned long tx_tout;
>>> +     bool knows_txdone;
>>> +     void *cntlr_data;
>>
>> What is the current use-case for exposing cntrl_data through ipc_client?
>> I think this should be avoided and leave the controller configuration to
>> the controller driver implementation. This will be a problem for
>> multiple link scenarios.
>>
> There are some highly programmable controllers that, if the driver is
> proper, could change behavior runtime. For ex, if the controller
> supports, a client might want to broadcast messages to multiple
> remotes or listen for messages from more than one remote source or
> both. The client could, say, specify bitmasks for such source/sink
> remotes via cntlr_data while requesting the mailbox.  PL320 could work
> in such mode if rest all fits.

Are you configuring the specific mailbox or configuring the controller
on a whole in this particular example?

> 
> 
>>> +struct ipc_link {
>>> +     char link_name[16];
>>> +     void *api_priv;
>>> +};
>>
>> this definitely needs a link-private void *ptr. PL320 doesn't have
>> multiple links in the controller, but this is a problem for OMAP & DBX500.
>>
> The API keeps the ipc_link provided by the controller driver, which
> could have it embedded in its private representation of the h/w links.
>  For ex, my controller has 6 links, and its structure contains 6
> element array of 'struct myhw_link' which embeds an ipc_link. And I
> use container_of().

Ofcourse, that works. But I was trying to avoid it, since it streamlines
the controller implementation better - it can avoid managing another
variable to pass the links just for registration (especially if I have
to dynamically create them), and also manage the creation/static
declaration of the actual links. The mailbox/ipc core is anyway
constructing a ipc_chan above this link, and doesn't use it directly for
anything.

> 
>>> +struct ipc_link_ops {
>>> +     int (*send_data)(struct ipc_link *link, void *data);
>>> +     int (*startup)(struct ipc_link *link, void *params);
>>> +     void (*shutdown)(struct ipc_link *link);
>>> +     bool (*last_tx_done)(struct ipc_link *link);
>> minor comment, maybe rename this to something that indicates link
>> busyness or readiness
>>
> The API gives the controller only one TX at a time, so I chose the
> name. What do you suggest?

how about 'is_ready', i think it is a bit more generic name and
automatically implies there are no pending tx.

>>> +};
>>> +
>>> +/**
>>> + * struct ipc_controller - Controller of a class of communication links
>>> + * @controller_name: Literal name of the controller.
>>> + * @ops: Operators that work on each communication link
>>> + * @links: Null terminated array of links.
>>> + * @txdone_irq: Indicates if the controller can report to API when the
>>> + *    last transmitted data was read by the remote. Eg, if it has some
>>> + *    TX ACK irq.
>>> + * @txdone_poll: If the controller can read but not report the TX done.
>>> + *    Eg, is some register shows the TX status but no interrupt rises.
>>> + *    Ignored if 'txdone_irq' is set.
>>> + * @txpoll_period: If 'txdone_poll' is in effect, the API polls for
>>> + *    last TX's status after these many millisecs
>>> + */
>>> +struct ipc_controller {
>>> +     char controller_name[16];
>> i think this can be avoided and use the underlying dev-name directly
>> based on the controller device. Adding a struct device pointer would
>> automatically allow you to use the name from the probe.
>>
> Ah ha... yes. Thanks.

Is your intention to leave the dev out to the controller driver specific
structure (atleast from the OMAP example code). I was asking for this to
be part of the ipc_controller structure, use a struct device *dev
instead of controller_name, and construct the controller name from the
dev_name.

> 
>>> +     struct ipc_link_ops *ops;
>> I think this should be a property of the individual link for
>> flexibility. Right now, it probably doesn't make a difference (as seen
>> from the current mailbox code), but the moment we have one link behaving
>> differently this will make the ops implementation a bit messy.
>>
>> I think we also need a controller-specific ops, to put common stuff
>> between multiple links within the controller.
>>
> A controller is assumed to be a bunch of identical links/mailboxes.
> However the cntlr_data could still help the client specify mode in
> which it wants a mailbox behave.

Yeah, assumptions hold true until an unfortunate SoC comes up with it
:). The TI AM3352 has one link that doesn't behave quite like the other
(I am hoping to avoid using it), so I am ok with it for now. Guess we
can always change it when a concrete need arises.

I prefer to see the controller_ops for startup/shutdown atleast. This
will allow the controller code state machine to be much better
maintained and make the code look good. As I said, with our new device,
where we have 13 controllers this will be very helpful. Also from what
Andy explained, you seem to have two controllers that are quite
different from each other.

regards
Suman

[RFC 2/3] mailbox: Introduce a new common API

[Jassi Brar]

Hi Suman,

On 7 May 2013 05:15, Suman Anna <s-anna@ti.com> wrote:
>>
>>  The client(s) can always generate TX requests at a rate greater than
>> the API could transmit on the physical link. So as much as we dislike
>> it, we have to buffer TX requests, otherwise N clients would.
>
> The current code doesn't support N clients today anyway, and if they are
> blocking mode on top of it, it would never need Tx buffering.
>
No, I meant if the API doesn't buffer TX requests, then N client
drivers that simply need to send 2 or more messages would have to
buffer them locally.
 Also by buffering TX requests the API reduces TX latency by
submitting the next request atomically as soon as it knows the last TX
is done.


>>
>> IMHO if clients on OMAP need to buffer RX, let us keep it OMAP
>> specific. If number of such platforms rise in future we could move
>> that as an _optional_ helper API on top, that does RX buffering on
>> behalf of clients ?
>
> Yeah, that's my idea as well to put this in the OMAP mailbox controller
> driver implementation. The IPC stacks on OMAP are quite established
> (both in remoteproc and DSP/Bridge - different generations), so I cannot
> afford to break the functionality of either of those. This already means
> I have to export some API from my controller driver and there is no
> other way or now.
>
Yes, I anticipated that.
Though imho OMAP's mailbox client/controller drivers could have be better.
 The controller has 8 instances of identical mailboxes/links which
operate independent of each other. It is the s/w implementation that
has tacit understanding of who sends/receives on which link. So
ideally a client driver should simply, say for omap3430, ask for
separate "mbox1" to read messages and "mbox0" to send messages to DSP.
 The unnecessary logical coupling of 0th and 1st links seems to be the
cause of 'hacks' like mailbox_dis/enable_irq(), whereas it should have
been release/request of the RX mbox.
Also logically coupling the independent physical links makes the
controller driver not very portable - what if some other platform has
usage of 3 send+receive and 2 send-only communications of the 8
physical links? The controller h/w is perfectly capable of providing
that, but its driver is not.
  Similarly something sure seems fishy about the client driver having
to explicitly save and restore registers of controller. Anyways, OMAP'
implementation of mailbox is a separate subject.


>>>
>>  If the API provides shared ownership of a mailbox, it won't work for
>> clients that need exclusive ownership (like 'server' side
>> implementation of a protocol).
>>  OTOH if the API provides exclusive ownership, it is still possible to
>> emulate shared ownership by simply publishing the mailbox handle (void
>> *chan) globally. It works for all.
>
> I am not saying provide this always. Have this dictated by the
> controller or mailbox (look at my branch). The global publication works
> well functionality-wise for Tx, but not so much for Rx.
>
Right now for RX the clients register a blocking notifier call within
the API, you could move that out into the same code that publishes the
TX handlle globally.  I am a bit hesitant because I want to make sure
we don't push anything into the common API that isn't already done
right in OMAP's implementation.

> In anycase,
> global publication will have its own set of problems - mostly you are
> implying another layer of implementation that provides this sharing
> capability (since they have to be outside of any single client).
>
Yes. Though yet another way of doing it is for the controller driver
to populate N logical per physical link that needs to be shared.

BTW, there doesn't exist any code/dt that creates the "omap-rproc"
platform_device so I may have misunderstood, but it seems the OMAP
already acquires a mailbox only once and save it in the data structure
representing the remote, which should simply work as such. No?


>>>
>> Most of the clients won't queue more than 1 request at a time. And
>> then, isn't it only natural that clients don't mess with requests
>> after submitting them ?  I see mailbox clients working quite like I2C
>> clients.
>
> The OMAP use-cases do this today, so not a good assumption to make. The
> OMAP IPC is built upon simple transmitting and processing received
> messages separately, we will not be using blocking mode or tx callbacks.
> I really don't need the complexity of having to introduce tx callbacks
> just because I need to know if my pointer can be freed, as I said - it
> eliminates the simplicity of using local variables unless the send API
> is blocking. void * works for OMAP since my payloads are only 4 bytes by
> themselves (this is a discussion that came up during the earlier series
> as well), but this is not flexible enough on the clients-side generally.
>
The OMAP simply pass by value a u32 as a message.  Why do you think it
won't work ? (Please have a look at my implementation of the omap2
controller driver)


>>
>>> Secondly, we may not need the logic around client knows_txdone and
>>> tx_buffering together. I understand the need for buffering when you have
>>> TX_POLL or TX_IRQ, but as far as TX_ACK is concerned, it is a client
>>> protocol-level knowledge, and so the tx buffering logic can remain with
>>> the client itself. This should simplify the code a little bit and get
>>> rid of the ipc_client_txdone API.
>>>
>> Yeah I had it that way originally. But then I realized we could be
>> running an 'ACK Packet' protocol over a controller that supports only
>> POLL. In that case the controller's poll doesn't override client's
>> knows_txdone because we want the client to cut-short the next poll
>> delay as soon as it gets an ACK packet.
>
> I think the same can be achieved from the Rx path upon an interrupt in
> the specific controller, without having to introduce the knows_txdone.
>
RX may not only be responses, it could very well be a remote command
just before the response to the last local command. OR the RX
responses may be out of order. So it can't be tied to the RX
interrupt.

> It is knowledge intrinsic to a controller/mailbox. I really hope that
> there are no controllers where you have to poll for Rx too :)
>
No, not intrinsic to controller, but to the high level protocol.
Yes, I assume the RX to be always interrupt driven :)

>>
>>> Looking at the current use-cases, I think OMAP might be the only one
>>> which needs the buffering. The API that Loic added suggests that he
>>> either sends a message and expects a response, or just sends a message
>>> if the transport is free (I am not sure if he is using the API that uses
>>> buffering currently). PL320 is the same as the first scenario that Loic has.
>>>
>> Yeah I too expect TX buffering to be very rare.
>
> If so, can we eliminate this for the first pass, and leave it to the
> controllers for now or dictate this based on a tx_buffering property? As
> I see it, we do not yet have an agreement on the Tx buffering semantics.
>
As I said, among other advantages, TX buffering is needed to reduce
latencies. My platform's protocol isn't yet clear as to whether use
shared-memory (secure and non-secure versions) for not very large data
transfers or interrupt payload. So if we get good enough speed we
could save cost on dedicated memories.

I wrote the omap2 controller driver code to convey my proposal better.
If you could please tell me how TX buffering is a problem for OMAP, it
will help me understand your concern better.

>>>
>> First of all, PL320 is a highly configurable and programmable IP.
>> pl320.c is very specific to Highbank's usage, it is not usable as such
>> on other platforms. There are many things hardcoded and tacitly
>> assumed in the driver. I just changed the API, didn't add anything
>> new.
>
> OK, should we be renaming this file accordingly then until another
> common user comes along? Also, an appropriate 'depends on' in the Kconfig.
>
Yes, will do. Thanks.


>>
>> Secondly, I believe we acknowledged the fact that a client can't do
>> without controller specific knowledge. So in proper implementation the
>> controller's header would look like
>>
>> struct foobar_con_mssg {
>>    int datalen; /* size of received packet in bytes */
>>    u8 *buf;  /* buffer that holds the packet data */
>>     .....
>> };
>>
>> So instead of telling the API the 'datalen' and the remaining
>> structure of each message, we simply pass "struct foobar_con_mssg *"
>> as a void* to the API.
>
> This has to be documented very well. And yes, I did acknowledge the
> inter-dependencies portion for understanding the packet structure (there
> is no way if you have a packet of 6 ints and you expect specific ints
> for specific fields). The size and void * are generic enough that can be
> present in the core to perform some error checking. The API would be
> akin to memcpy then. The other motivation is based on the buffering
> scheme (same comment as above on Tx buffering), which we haven't agreed
> upon completely.
>
How could the core perform any meaningful sanity check?  What if we
send/receive proper 'datalen' of garbage data?  What if the client
sends a "Give me info in not more than 32-bytes at a time" command to
the remote and the remote has valid info worth only 16 bytes?
IMHO there is not much for core to verify in a packet in transit. The
introduced checks are not worth it and actually limit some scenarios
like above example.


>>>
>>> Assigning the tx_block at channel request time might not be flexible
>>> enough. We should not impose that the client will request and release
>>> channels with different modes if both scenarios are needed.
>>>
>> If there is one blocking request, no more could arrive (blocking or
>> non-blocking).
>
> Well, you are assuming that there is only one thread from within the
> client driver who is gonna use the send API. There is nothing in the
> code that stops from say two threads/contexts queueing in the message
> from the same client driver. I am ok from my side for this to be sorted
> out later, since I won't be using the blocking mode,
>
Thanks


>>>
>>> Do we need to add an API similar to mailbox_msg_send_receive_no_irq, or
>>> are you leaving it to be client's responsibility?
>>>
>> Yes the API provides enough ways for a client to do that.
>
> I will let Loic comment on this, based on his usage/complexity in the
> client. I think you got lucky with PL320 implementation since it fills
> in the buffer on a Tx done, so you have folded that functionality in
> send_data itself.
>
No, the pl320 models the links properly. The links could be used for
RX or TX independently.
Any RX channel will return data only from a remote. The TX returns
data in tx-buffer which may or may not be changed by the remote after
reading. The Highbank's protocol make use of that feature. Though I am
interested to know if Loic has some unmet requirements still.


>>>
>> Yeah, I changed that to it last minute :)
>> The API has no real use of controller nodes, it works solely on a
>> global pool of mailboxes. I tried to reflect that in code.
>
> Can we go back to using controller nodes? I understand that the API
> doesn't use much, but the list of controller nodes is gonna be static
> anyway in the file. It just improves the search algorithm when looking
> for a mailbox. We have a new device coming up (u-boot patches already
> submitted) wherein we have 13 controller instances, each with about 4 to
> 6 links, a linear search would be really painful.
>
OK I'll keep a local list of controllers.


>>>
>> There are some highly programmable controllers that, if the driver is
>> proper, could change behavior runtime. For ex, if the controller
>> supports, a client might want to broadcast messages to multiple
>> remotes or listen for messages from more than one remote source or
>> both. The client could, say, specify bitmasks for such source/sink
>> remotes via cntlr_data while requesting the mailbox.  PL320 could work
>> in such mode if rest all fits.
>
> Are you configuring the specific mailbox or configuring the controller
> on a whole in this particular example?
>
The cntlr_data is to configure the link/mailbox, not the controller.

>>>
>> The API gives the controller only one TX at a time, so I chose the
>> name. What do you suggest?
>
> how about 'is_ready', i think it is a bit more generic name and
> automatically implies there are no pending tx.
>
OK I can live with that ;)

>
> Is your intention to leave the dev out to the controller driver specific
> structure (atleast from the OMAP example code). I was asking for this to
> be part of the ipc_controller structure, use a struct device *dev
> instead of controller_name, and construct the controller name from the
> dev_name.
>
Yes, the ipc_controller should have it. I didn't implement that yet in
v2 though.

>>>
>> A controller is assumed to be a bunch of identical links/mailboxes.
>> However the cntlr_data could still help the client specify mode in
>> which it wants a mailbox behave.
>
> Yeah, assumptions hold true until an unfortunate SoC comes up with it
> :). The TI AM3352 has one link that doesn't behave quite like the other
> (I am hoping to avoid using it), so I am ok with it for now. Guess we
> can always change it when a concrete need arises.
>
It would work right now :)   If your controller has 2 types of links,
the driver should register each bunch separately, each with its own
ipc_link_ops, with the core. The core can already take care of the
rest.


> I prefer to see the controller_ops for startup/shutdown atleast. This
> will allow the controller code state machine to be much better
> maintained and make the code look good. As I said, with our new device,
> where we have 13 controllers this will be very helpful. Also from what
> Andy explained, you seem to have two controllers that are quite
> different from each other.
>
Yes, I have 2 types of controllers and I already thought about
multiple controllers in a platform.
 What do you expect to do in controller startup/shutdown? Since the
API works on individual links, not the controllers, when would it call
controller_start() and controller_shutdown()?  The ipc_link's
startup/shutdown are already non-atomic, if the controller needs any
setup it could do it in the first link's startup and teardown in the
last link's shutdown. Not to forget the resources lke IRQ are usually
per Link which should be acquired/released upon link's use/idle.
 While writing the OMAP2 controller driver, did I miss any controller
startup/shutdown ?

Regards,
-Jassi

[RFC 2/3] mailbox: Introduce a new common API

[Suman Anna]

Hi Jassi,

> On 7 May 2013 05:15, Suman Anna <s-anna@ti.com> wrote:
>>>
>>>  The client(s) can always generate TX requests at a rate greater than
>>> the API could transmit on the physical link. So as much as we dislike
>>> it, we have to buffer TX requests, otherwise N clients would.
>>
>> The current code doesn't support N clients today anyway, and if they are
>> blocking mode on top of it, it would never need Tx buffering.
>>
> No, I meant if the API doesn't buffer TX requests, then N client
> drivers that simply need to send 2 or more messages would have to
> buffer them locally.
>  Also by buffering TX requests the API reduces TX latency by
> submitting the next request atomically as soon as it knows the last TX
> is done.

OK. I do not have an issue with Tx buffering, but the manner it is done.
As I said before, there is a big implementation detail on client users
keeping their pointers in tact until the actual Tx is done - it
increases the complexity of client code. I have proposed an idea below
on the size discussion.

>>>
>>> IMHO if clients on OMAP need to buffer RX, let us keep it OMAP
>>> specific. If number of such platforms rise in future we could move
>>> that as an _optional_ helper API on top, that does RX buffering on
>>> behalf of clients ?
>>
>> Yeah, that's my idea as well to put this in the OMAP mailbox controller
>> driver implementation. The IPC stacks on OMAP are quite established
>> (both in remoteproc and DSP/Bridge - different generations), so I cannot
>> afford to break the functionality of either of those. This already means
>> I have to export some API from my controller driver and there is no
>> other way or now.
>>
> Yes, I anticipated that.
> Though imho OMAP's mailbox client/controller drivers could have be better.
>  The controller has 8 instances of identical mailboxes/links which
> operate independent of each other. It is the s/w implementation that
> has tacit understanding of who sends/receives on which link. So
> ideally a client driver should simply, say for omap3430, ask for
> separate "mbox1" to read messages and "mbox0" to send messages to DSP.

I think this is a good example for link_ops specified with the link,
rather than the controller (send_data would be NULL on the Rx instance
for example). In anycase, there is not much to be bought implementing
this way on OMAP (maybe sometime in the far future if we assign receive
queues by processor, rather than defining the pair as a communication
link with some message decoding indicating tx processor), since our
remotes are processors and the link assignment configuration doesn't
change dynamically.

>  The unnecessary logical coupling of 0th and 1st links seems to be the
> cause of 'hacks' like mailbox_dis/enable_irq(), whereas it should have
> been release/request of the RX mbox.

The logical coupling is not the reason, but that's because of the IPC
protocol stack implementation in DSP/Bridge, that's a story for another
day. remoteproc does not suffer from this.

> Also logically coupling the independent physical links makes the
> controller driver not very portable - what if some other platform has
> usage of 3 send+receive and 2 send-only communications of the 8
> physical links? The controller h/w is perfectly capable of providing
> that, but its driver is not.

That's upto that controller driver, right?

>   Similarly something sure seems fishy about the client driver having
> to explicitly save and restore registers of controller. Anyways, OMAP'
> implementation of mailbox is a separate subject.

Yeah, not too concerned here, the save/restore part can be fixed rather
easily. I will look into that once we get the series and the initial
migration done.

>>>>
>>>  If the API provides shared ownership of a mailbox, it won't work for
>>> clients that need exclusive ownership (like 'server' side
>>> implementation of a protocol).
>>>  OTOH if the API provides exclusive ownership, it is still possible to
>>> emulate shared ownership by simply publishing the mailbox handle (void
>>> *chan) globally. It works for all.
>>
>> I am not saying provide this always. Have this dictated by the
>> controller or mailbox (look at my branch). The global publication works
>> well functionality-wise for Tx, but not so much for Rx.
>>
> Right now for RX the clients register a blocking notifier call within
> the API, you could move that out into the same code that publishes the
> TX handlle globally.  I am a bit hesitant because I want to make sure
> we don't push anything into the common API that isn't already done
> right in OMAP's implementation.
> 
>> In anycase,
>> global publication will have its own set of problems - mostly you are
>> implying another layer of implementation that provides this sharing
>> capability (since they have to be outside of any single client).
>>
> Yes. Though yet another way of doing it is for the controller driver
> to populate N logical per physical link that needs to be shared.

Yeah, I was implying the same idea.. The notifier chains in the current
driver really meant the logical and physical is the same as we are
returning the link handle, but with your current design of returning
ipc_chan as an opaque handle, you already have the base infrastructure
in place. Once we add tx callbacks, even the original mailbox series
would have transformed into having a similar concept as what you have.

> 
> BTW, there doesn't exist any code/dt that creates the "omap-rproc"
> platform_device so I may have misunderstood, but it seems the OMAP
> already acquires a mailbox only once and save it in the data structure
> representing the remote, which should simply work as such. No?

Yeah, the rproc device creation code is currently out of tree (you can
find it in Ohad's remoteproc tree on git.kernel.org), it is currently
disabled due to the lack of multi-platform support in mailbox
(dependencies on this series and other DT conversions). The runtime
support would have relinquished the mailboxes. remoteproc already looks
in certain messages for device management, and others it passes onto the
rpmsg bus, but that is rather tightly coupled.

> 
> 
>>>>
>>> Most of the clients won't queue more than 1 request at a time. And
>>> then, isn't it only natural that clients don't mess with requests
>>> after submitting them ?  I see mailbox clients working quite like I2C
>>> clients.
>>
>> The OMAP use-cases do this today, so not a good assumption to make. The
>> OMAP IPC is built upon simple transmitting and processing received
>> messages separately, we will not be using blocking mode or tx callbacks.
>> I really don't need the complexity of having to introduce tx callbacks
>> just because I need to know if my pointer can be freed, as I said - it
>> eliminates the simplicity of using local variables unless the send API
>> is blocking. void * works for OMAP since my payloads are only 4 bytes by
>> themselves (this is a discussion that came up during the earlier series
>> as well), but this is not flexible enough on the clients-side generally.
>>
> The OMAP simply pass by value a u32 as a message.  Why do you think it
> won't work ? (Please have a look at my implementation of the omap2
> controller driver)

I am talking about queueing more than 1 request, we simply send a
message that can be triggered by either remoteproc for PM purposes or by
different rpmsg clients sending a message. It is delivered into the
mailbox and there is no implicit ack needed on tx_done since that is
dealt by the client messaging protocol sitting above. We will not be
using blocking mode, so there can be multiple requests at the same time
(in the queue).

> 
>>>
>>>> Secondly, we may not need the logic around client knows_txdone and
>>>> tx_buffering together. I understand the need for buffering when you have
>>>> TX_POLL or TX_IRQ, but as far as TX_ACK is concerned, it is a client
>>>> protocol-level knowledge, and so the tx buffering logic can remain with
>>>> the client itself. This should simplify the code a little bit and get
>>>> rid of the ipc_client_txdone API.
>>>>
>>> Yeah I had it that way originally. But then I realized we could be
>>> running an 'ACK Packet' protocol over a controller that supports only
>>> POLL. In that case the controller's poll doesn't override client's
>>> knows_txdone because we want the client to cut-short the next poll
>>> delay as soon as it gets an ACK packet.
>>
>> I think the same can be achieved from the Rx path upon an interrupt in
>> the specific controller, without having to introduce the knows_txdone.
>>
> RX may not only be responses, it could very well be a remote command
> just before the response to the last local command. OR the RX
> responses may be out of order. So it can't be tied to the RX
> interrupt.

Do you have the same shared payload in both directions? If so, the above
will work. Otherwise, I understand the scenario.

>> It is knowledge intrinsic to a controller/mailbox. I really hope that
>> there are no controllers where you have to poll for Rx too :)
>>
> No, not intrinsic to controller, but to the high level protocol.
> Yes, I assume the RX to be always interrupt driven :)

Until someone (out of their minds) comes up with it :)

> 
>>>
>>>> Looking at the current use-cases, I think OMAP might be the only one
>>>> which needs the buffering. The API that Loic added suggests that he
>>>> either sends a message and expects a response, or just sends a message
>>>> if the transport is free (I am not sure if he is using the API that uses
>>>> buffering currently). PL320 is the same as the first scenario that Loic has.
>>>>
>>> Yeah I too expect TX buffering to be very rare.
>>
>> If so, can we eliminate this for the first pass, and leave it to the
>> controllers for now or dictate this based on a tx_buffering property? As
>> I see it, we do not yet have an agreement on the Tx buffering semantics.
>>
> As I said, among other advantages, TX buffering is needed to reduce
> latencies. My platform's protocol isn't yet clear as to whether use
> shared-memory (secure and non-secure versions) for not very large data
> transfers or interrupt payload. So if we get good enough speed we
> could save cost on dedicated memories.
> 
> I wrote the omap2 controller driver code to convey my proposal better.
> If you could please tell me how TX buffering is a problem for OMAP, it
> will help me understand your concern better.

It is not a problem for OMAP, it is about only maintaining the void * in
the Tx queue. Since OMAP uses only a u32 message, I could have used the
void * as the u32 message but not a pointer (same approach we had taken
in the previous series). I am talking in general about the void *, and
the limitations it poses on client users. The other thing is that you
have defined 10 as the queue length in the core, this should be dictated
by the controller or mailbox. What works for one platform may not work
for some others (this is what you saw as different config options
between OMAP and DBX500)


>>>
>>> Secondly, I believe we acknowledged the fact that a client can't do
>>> without controller specific knowledge. So in proper implementation the
>>> controller's header would look like
>>>
>>> struct foobar_con_mssg {
>>>    int datalen; /* size of received packet in bytes */
>>>    u8 *buf;  /* buffer that holds the packet data */
>>>     .....
>>> };
>>>
>>> So instead of telling the API the 'datalen' and the remaining
>>> structure of each message, we simply pass "struct foobar_con_mssg *"
>>> as a void* to the API.
>>
>> This has to be documented very well. And yes, I did acknowledge the
>> inter-dependencies portion for understanding the packet structure (there
>> is no way if you have a packet of 6 ints and you expect specific ints
>> for specific fields). The size and void * are generic enough that can be
>> present in the core to perform some error checking. The API would be
>> akin to memcpy then. The other motivation is based on the buffering
>> scheme (same comment as above on Tx buffering), which we haven't agreed
>> upon completely.
>>
> How could the core perform any meaningful sanity check?  What if we
> send/receive proper 'datalen' of garbage data?  What if the client
> sends a "Give me info in not more than 32-bytes at a time" command to
> the remote and the remote has valid info worth only 16 bytes?
> IMHO there is not much for core to verify in a packet in transit. The
> introduced checks are not worth it and actually limit some scenarios
> like above example.

OK, I had mainly two ideas, one telling the size of the void * on Tx,
with the controller driver specifying max (and maybe min sizes expected)
so that some base error check can be done. This may be a basic check,
but atleast provides some level of sanity checking. On Rx-side, send the
size of the void * back to the client driver. The example you gave is
upto the client communication protocol, the remote may not even choose
to send the data if it doesn't have 32 bytes.

The second (major reason) idea was driven by the kfifo implementation
for buffering in core code for which I need the size field, so that the
clients need not be intelligent about the status of its tx submission
and maintaining its pointer (if you have different calling contexts). I
guess both of us are coming from different perspectives here, you may
find kfifos inefficient and I may find the complexity in client driver
unnecessary for maintaining pointers. How about defining add and remove
buffer ops in the controller driver so that it can choose its
implementation, while we keep the overall flow architecture in the core
code?

> 
> 
>>>>
>>>> Assigning the tx_block at channel request time might not be flexible
>>>> enough. We should not impose that the client will request and release
>>>> channels with different modes if both scenarios are needed.
>>>>
>>> If there is one blocking request, no more could arrive (blocking or
>>> non-blocking).
>>
>> Well, you are assuming that there is only one thread from within the
>> client driver who is gonna use the send API. There is nothing in the
>> code that stops from say two threads/contexts queueing in the message
>> from the same client driver. I am ok from my side for this to be sorted
>> out later, since I won't be using the blocking mode,
>>
> Thanks
> 
>>>>
>>>> Do we need to add an API similar to mailbox_msg_send_receive_no_irq, or
>>>> are you leaving it to be client's responsibility?
>>>>
>>> Yes the API provides enough ways for a client to do that.
>>
>> I will let Loic comment on this, based on his usage/complexity in the
>> client. I think you got lucky with PL320 implementation since it fills
>> in the buffer on a Tx done, so you have folded that functionality in
>> send_data itself.
>>
> No, the pl320 models the links properly. The links could be used for
> RX or TX independently.
> Any RX channel will return data only from a remote. The TX returns
> data in tx-buffer which may or may not be changed by the remote after
> reading. The Highbank's protocol make use of that feature. Though I am
> interested to know if Loic has some unmet requirements still.

thanks for the clarification.

> 
> 
>>>>
>>> Yeah, I changed that to it last minute :)
>>> The API has no real use of controller nodes, it works solely on a
>>> global pool of mailboxes. I tried to reflect that in code.
>>
>> Can we go back to using controller nodes? I understand that the API
>> doesn't use much, but the list of controller nodes is gonna be static
>> anyway in the file. It just improves the search algorithm when looking
>> for a mailbox. We have a new device coming up (u-boot patches already
>> submitted) wherein we have 13 controller instances, each with about 4 to
>> 6 links, a linear search would be really painful.
>>
> OK I'll keep a local list of controllers.

Sure, thanks.

> 
> 
>>>>
>>> There are some highly programmable controllers that, if the driver is
>>> proper, could change behavior runtime. For ex, if the controller
>>> supports, a client might want to broadcast messages to multiple
>>> remotes or listen for messages from more than one remote source or
>>> both. The client could, say, specify bitmasks for such source/sink
>>> remotes via cntlr_data while requesting the mailbox.  PL320 could work
>>> in such mode if rest all fits.
>>
>> Are you configuring the specific mailbox or configuring the controller
>> on a whole in this particular example?
>>
> The cntlr_data is to configure the link/mailbox, not the controller.

then, can we rename this to 'link_data' or something that doesn't imply
controller to avoid confusion? I guess you meant it as controller driver
data rather than controller instance data, but the name is a bit misleading.

> 
>>>>
>>> A controller is assumed to be a bunch of identical links/mailboxes.
>>> However the cntlr_data could still help the client specify mode in
>>> which it wants a mailbox behave.
>>
>> Yeah, assumptions hold true until an unfortunate SoC comes up with it
>> :). The TI AM3352 has one link that doesn't behave quite like the other
>> (I am hoping to avoid using it), so I am ok with it for now. Guess we
>> can always change it when a concrete need arises.
>>
> It would work right now :)   If your controller has 2 types of links,
> the driver should register each bunch separately, each with its own
> ipc_link_ops, with the core. The core can already take care of the
> rest.

Yeah, it can be done that way, but that's a not-so-elegant work-around
IMHO. It's calling ipc_links_register twice on the same controller. Idea
was that the API would distinguish different controllers, not the same
one. Also, see the above example if you were to treat a link as Rx or Tx
only (functional behavior differences even though the link is exactly
the same type).

> 
>> I prefer to see the controller_ops for startup/shutdown atleast. This
>> will allow the controller code state machine to be much better
>> maintained and make the code look good. As I said, with our new device,
>> where we have 13 controllers this will be very helpful. Also from what
>> Andy explained, you seem to have two controllers that are quite
>> different from each other.
>>
> Yes, I have 2 types of controllers and I already thought about
> multiple controllers in a platform.
>  What do you expect to do in controller startup/shutdown? Since the
> API works on individual links, not the controllers, when would it call
> controller_start() and controller_shutdown()?  The ipc_link's
> startup/shutdown are already non-atomic, if the controller needs any
> setup it could do it in the first link's startup and teardown in the
> last link's shutdown. Not to forget the resources lke IRQ are usually
> per Link which should be acquired/released upon link's use/idle.
>  While writing the OMAP2 controller driver, did I miss any controller
> startup/shutdown ?

It just delineates the code better, and has flexibility later on to
extend any controller ops or exposing new controller-specific API. The
controller start and stop will be called in the same function as
ipc_request_channel. The other thing is adding a check for the presence
of startup ops before actually invoking it, that way controller
implementations with no special startup needed can avoid it. Controller
ops in ipc_controller and link_ops in ipc_link would give better
flexibility for controller driver implementations.

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.

regards
Suman

[RFC 2/3] mailbox: Introduce a new common API

[Jassi Brar]

On 8 May 2013 03:18, Suman Anna <s-anna@ti.com> wrote:
> Hi Jassi,
>
>> On 7 May 2013 05:15, Suman Anna <s-anna@ti.com> wrote:
>>>>
>>>>  The client(s) can always generate TX requests at a rate greater than
>>>> the API could transmit on the physical link. So as much as we dislike
>>>> it, we have to buffer TX requests, otherwise N clients would.
>>>
>>> The current code doesn't support N clients today anyway, and if they are
>>> blocking mode on top of it, it would never need Tx buffering.
>>>
>> No, I meant if the API doesn't buffer TX requests, then N client
>> drivers that simply need to send 2 or more messages would have to
>> buffer them locally.
>>  Also by buffering TX requests the API reduces TX latency by
>> submitting the next request atomically as soon as it knows the last TX
>> is done.
>
> OK. I do not have an issue with Tx buffering,
>
OK, thanks for confirming.

> but the manner it is done.
> As I said before, there is a big implementation detail on client users
> keeping their pointers in tact until the actual Tx is done - it
> increases the complexity of client code. I have proposed an idea below
> on the size discussion.
>
What makes this API different that you want it to create internal
copies of clients' requests?
I2C clients have been working fine. I believe most other apis work
similarly. No client died of the burden of having to preserve request
pointers until the request is served by the API.

BTW, by forcing the API to not directly use request structures from
clients we kill chances of zero-copy transfer use-cases.

Sorry, I don't think just to make it "easy" for some rare client (that
issues non-blocking pass-by-reference requests and don't even care to
know if the message was ever sent), we should add complexity to the
API _and_ kill some important use-cases.


>> Also logically coupling the independent physical links makes the
>> controller driver not very portable - what if some other platform has
>> usage of 3 send+receive and 2 send-only communications of the 8
>> physical links? The controller h/w is perfectly capable of providing
>> that, but its driver is not.
>
> That's upto that controller driver, right?
>
Yes, I meant the OMAP's controller driver need to be re-written
properly so as to not couple 2 links together as one RX and one TX.


>>> In anycase,
>>> global publication will have its own set of problems - mostly you are
>>> implying another layer of implementation that provides this sharing
>>> capability (since they have to be outside of any single client).
>>>
>> Yes. Though yet another way of doing it is for the controller driver
>> to populate N logical per physical link that needs to be shared.
>
> Yeah, I was implying the same idea.. The notifier chains in the current
> driver really meant the logical and physical is the same as we are
> returning the link handle, but with your current design of returning
> ipc_chan as an opaque handle, you already have the base infrastructure
> in place. Once we add tx callbacks, even the original mailbox series
> would have transformed into having a similar concept as what you have.
>
Though I would still suggest you manage a remote by a local 'server'
code - not every client needs to be bothered with request/response for
every other client.
Also the OMAP might be lucky, but the remote may not always simply
respond to commands from local clients. The remote might also send
some SOS or similar command on behalf of the remote as a whole. Such
requests should be handled by that local 'server' code.


>> The OMAP simply pass by value a u32 as a message.  Why do you think it
>> won't work ? (Please have a look at my implementation of the omap2
>> controller driver)
>
> I am talking about queueing more than 1 request, we simply send a
> message that can be triggered by either remoteproc for PM purposes or by
> different rpmsg clients sending a message. It is delivered into the
> mailbox and there is no implicit ack needed on tx_done since that is
> dealt by the client messaging protocol sitting above. We will not be
> using blocking mode, so there can be multiple requests at the same time
> (in the queue).
>
Yes that's what I am talking about.  Multiple requests would still be
a u32 each.

Since for OMAP a message is just a 32-bit number, a client would queue
requests as
{
      u32 cmd;
      cmd = COMMAND_A;
      ipc_send_message(chan, (void *)cmd);
      cmd = COMMAND_B;
      ipc_send_message(chan, (void *)cmd);
      cmd = COMMAND_C;
      ipc_send_message(chan, (void *)cmd);
}

The controller driver retrieves each message from the API and transmit as
       writel((u32) voidptr_cmd,  txreg);

How could it get any simpler ?


>>> It is knowledge intrinsic to a controller/mailbox. I really hope that
>>> there are no controllers where you have to poll for Rx too :)
>>>
>> No, not intrinsic to controller, but to the high level protocol.
>> Yes, I assume the RX to be always interrupt driven :)
>
> Until someone (out of their minds) comes up with it :)
>
OR someone smart tries to run some IPC stack over, say, gpio bitbanging ;)


>>>
>> As I said, among other advantages, TX buffering is needed to reduce
>> latencies. My platform's protocol isn't yet clear as to whether use
>> shared-memory (secure and non-secure versions) for not very large data
>> transfers or interrupt payload. So if we get good enough speed we
>> could save cost on dedicated memories.
>>
>> I wrote the omap2 controller driver code to convey my proposal better.
>> If you could please tell me how TX buffering is a problem for OMAP, it
>> will help me understand your concern better.
>
> It is not a problem for OMAP
>
Thanks for confirming that it's not a problem for OMAP.


>  The other thing is that you
> have defined 10 as the queue length in the core, this should be dictated
> by the controller or mailbox. What works for one platform may not work
> for some others (this is what you saw as different config options
> between OMAP and DBX500)
>
Please read the multi-line comment over that single line define.
The buffer is cheap, just a void*, so we could make it even 50.
However I am not hung on that.
It purely depends upon how the client uses the link, so it can't be
driven by the controller. We could make it a Kconfig option. What do
you suggest ?


>>>
>> How could the core perform any meaningful sanity check?  What if we
>> send/receive proper 'datalen' of garbage data?  What if the client
>> sends a "Give me info in not more than 32-bytes at a time" command to
>> the remote and the remote has valid info worth only 16 bytes?
>> IMHO there is not much for core to verify in a packet in transit. The
>> introduced checks are not worth it and actually limit some scenarios
>> like above example.
>
> OK, I had mainly two ideas, one telling the size of the void * on Tx,
> with the controller driver specifying max (and maybe min sizes expected)
>
No, please. The controller driver is supposed to be reusable over
various protocols. A protocol dictates the length of each logical TX
unit. The max-min limits set by the controller driver might work for
one but fail some other higher level protocol.

>
> The second (major reason) idea was driven by the kfifo implementation
> for buffering in core code for which I need the size field, so that the
> clients need not be intelligent about the status of its tx submission
> and maintaining its pointer (if you have different calling contexts). I
> guess both of us are coming from different perspectives here, you may
> find kfifos inefficient and I may find the complexity in client driver
> unnecessary for maintaining pointers. How about defining add and remove
> buffer ops in the controller driver so that it can choose its
> implementation, while we keep the overall flow architecture in the core
> code?
>
As I said the API shouldn't keep internal copies of a client's
requests - it will only kill zero-copy use-cases while adding
complexity to the core.

Since it already works fine for OMAP, I request we leave it as such
for now at least.


>>>> There are some highly programmable controllers that, if the driver is
>>>> proper, could change behavior runtime. For ex, if the controller
>>>> supports, a client might want to broadcast messages to multiple
>>>> remotes or listen for messages from more than one remote source or
>>>> both. The client could, say, specify bitmasks for such source/sink
>>>> remotes via cntlr_data while requesting the mailbox.  PL320 could work
>>>> in such mode if rest all fits.
>>>
>>> Are you configuring the specific mailbox or configuring the controller
>>> on a whole in this particular example?
>>>
>> The cntlr_data is to configure the link/mailbox, not the controller.
>
> then, can we rename this to 'link_data' or something that doesn't imply
> controller to avoid confusion? I guess you meant it as controller driver
> data rather than controller instance data, but the name is a bit misleading.
>
OK, I'll call it  'link_data'  :)


>>>
>>> Yeah, assumptions hold true until an unfortunate SoC comes up with it
>>> :). The TI AM3352 has one link that doesn't behave quite like the other
>>> (I am hoping to avoid using it), so I am ok with it for now. Guess we
>>> can always change it when a concrete need arises.
>>>
>> It would work right now :)   If your controller has 2 types of links,
>> the driver should register each bunch separately, each with its own
>> ipc_link_ops, with the core. The core can already take care of the
>> rest.
>
> Yeah, it can be done that way, but that's a not-so-elegant work-around
>
OTOH I find it elegant and not a "work around".
Liberally estimating, hardly 10% of IPC controllers would have 2
classes of physical links. Having link_ops per controller reduces the
number of ipc_link_ops structures 11:20 as compared to per link.


> IMHO. It's calling ipc_links_register twice on the same controller. Idea
> was that the API would distinguish different controllers, not the same
> one. Also, see the above example if you were to treat a link as Rx or Tx
> only (functional behavior differences even though the link is exactly
> the same type).
>
Why is calling ipc_links_register() twice, for a controller with 2
classes of links, a problem ?

And, No, the API need not know if the link is "RX" or "TX", which is
purely a logical function.

Please do tell me which controller physically limits its links to do
only "RX" or only "TX"?  It's just the platform's firmware that
dictates who sends and who receives on which link, thereby giving the
illusion of a link being a "RX" or "TX" capable only.
Even if some h/w limits do exist on links of some controller, still
the API shouldn't try to discern a "RX" from a "TX" link. When a
client knows which link to open, it will also know if it should read
or write to it - this assumption already holds true. If the client
writes on a link that it's supposed to listen to remote on, no amount
of policing by the API could help.

Even if the API tries to differentiate "RX" and "TX" links, the
knowledge must come from a client (it's driven by the protocol on the
platform), and not the controller because each of its links are
physically equally capable to do RX or TX on each end.


>> Yes, I have 2 types of controllers and I already thought about
>> multiple controllers in a platform.
>>  What do you expect to do in controller startup/shutdown? Since the
>> API works on individual links, not the controllers, when would it call
>> controller_start() and controller_shutdown()?  The ipc_link's
>> startup/shutdown are already non-atomic, if the controller needs any
>> setup it could do it in the first link's startup and teardown in the
>> last link's shutdown. Not to forget the resources lke IRQ are usually
>> per Link which should be acquired/released upon link's use/idle.
>>  While writing the OMAP2 controller driver, did I miss any controller
>> startup/shutdown ?
>
> It just delineates the code better, and has flexibility later on to
> extend any controller ops or exposing new controller-specific API. The
> controller start and stop will be called in the same function as
> ipc_request_channel.
>
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();

Why have 2 contiguous calls to the controller, to perform similar
functions?  Not to forget the API and clients don't work on IPC
controllers, but only on links. So it would be out-of-line for them to
have to worry about the controller.

>
> 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. Anyways, I am
not in favour of controller ops for other reasons mentioned above.

May I suggest, atm, we care only about things that don't work for OMAP
or STE. And leave the design issues for later when/if some real and
compelling enough need arises otherwise. I am afraid we are reaching
the point where this thread could be called, what Linus.T termed as,
"mental masxxxx"  :O

Regards,
-Jassi

[RFC 2/3] mailbox: Introduce a new common API

[Suman Anna]

Hi Jassi,

>>>>>
>>>>>  The client(s) can always generate TX requests at a rate greater than
>>>>> the API could transmit on the physical link. So as much as we dislike
>>>>> it, we have to buffer TX requests, otherwise N clients would.
>>>>
>>>> The current code doesn't support N clients today anyway, and if they are
>>>> blocking mode on top of it, it would never need Tx buffering.
>>>>
>>> No, I meant if the API doesn't buffer TX requests, then N client
>>> drivers that simply need to send 2 or more messages would have to
>>> buffer them locally.
>>>  Also by buffering TX requests the API reduces TX latency by
>>> submitting the next request atomically as soon as it knows the last TX
>>> is done.
>>
>> OK. I do not have an issue with Tx buffering,
>>
> OK, thanks for confirming.
> 
>> but the manner it is done.
>> As I said before, there is a big implementation detail on client users
>> keeping their pointers in tact until the actual Tx is done - it
>> increases the complexity of client code. I have proposed an idea below
>> on the size discussion.
>>
> What makes this API different that you want it to create internal
> copies of clients' requests?
> I2C clients have been working fine. I believe most other apis work
> similarly. No client died of the burden of having to preserve request
> pointers until the request is served by the API.

You are implying that every client does need to implement a tx_callback,
or needs to care for because of the design in the common core code. I
cannot use local variables in whatever function I am sending the
requests. The example you have given below will fail when the client
doesn't care about tx callbacks neither is blocking, if the entire
request is implemented as a function.

> 
> BTW, by forcing the API to not directly use request structures from
> clients we kill chances of zero-copy transfer use-cases.

I doubt true zero-copy transfers can be achieved for most controllers,
you will almost always copy the client payload into the physical
transport payload. For true zero-copy, you would have to give out the
ioremapped transport payload ptr to a client.

> Sorry, I don't think just to make it "easy" for some rare client (that
> issues non-blocking pass-by-reference requests and don't even care to
> know if the message was ever sent), we should add complexity to the
> API _and_ kill some important use-cases.

The OMAP usecase (existing now) uses this approach, and we got lucky
because our payload is u32 and we will overwrite the ptr with the actual
message value instead of using it as a ptr. The existing API will fail
if I just have these two conditions met by a client - no blocking and no
tx callbacks.

>>> Also logically coupling the independent physical links makes the
>>> controller driver not very portable - what if some other platform has
>>> usage of 3 send+receive and 2 send-only communications of the 8
>>> physical links? The controller h/w is perfectly capable of providing
>>> that, but its driver is not.
>>
>> That's upto that controller driver, right?
>>
> Yes, I meant the OMAP's controller driver need to be re-written
> properly so as to not couple 2 links together as one RX and one TX.

Maybe for a different type of controller, but OMAP definitely doesn't
need this additional complexity for the foreseeable future, neither in
the client nor in the controller driver.

>>>> In anycase,
>>>> global publication will have its own set of problems - mostly you are
>>>> implying another layer of implementation that provides this sharing
>>>> capability (since they have to be outside of any single client).
>>>>
>>> Yes. Though yet another way of doing it is for the controller driver
>>> to populate N logical per physical link that needs to be shared.
>>
>> Yeah, I was implying the same idea.. The notifier chains in the current
>> driver really meant the logical and physical is the same as we are
>> returning the link handle, but with your current design of returning
>> ipc_chan as an opaque handle, you already have the base infrastructure
>> in place. Once we add tx callbacks, even the original mailbox series
>> would have transformed into having a similar concept as what you have.
>>
> Though I would still suggest you manage a remote by a local 'server'
> code - not every client needs to be bothered with request/response for
> every other client.

That's a client protocol or functional integration aspect of the
clients, remote and controller driver on the particular SoC, right?

> Also the OMAP might be lucky, but the remote may not always simply
> respond to commands from local clients. The remote might also send
> some SOS or similar command on behalf of the remote as a whole. Such
> requests should be handled by that local 'server' code.
> 
> 
>>> The OMAP simply pass by value a u32 as a message.  Why do you think it
>>> won't work ? (Please have a look at my implementation of the omap2
>>> controller driver)
>>
>> I am talking about queueing more than 1 request, we simply send a
>> message that can be triggered by either remoteproc for PM purposes or by
>> different rpmsg clients sending a message. It is delivered into the
>> mailbox and there is no implicit ack needed on tx_done since that is
>> dealt by the client messaging protocol sitting above. We will not be
>> using blocking mode, so there can be multiple requests at the same time
>> (in the queue).
>>
> Yes that's what I am talking about.  Multiple requests would still be
> a u32 each.

This discussion came about from your statement, "Most of the clients
won't queue more than 1 request at a time. And then, isn't it only
natural that clients don't mess with requests after submitting them?  I
see mailbox clients working quite like I2C clients."

> Since for OMAP a message is just a 32-bit number, a client would queue
> requests as
> {
>       u32 cmd;
>       cmd = COMMAND_A;
>       ipc_send_message(chan, (void *)cmd);
>       cmd = COMMAND_B;
>       ipc_send_message(chan, (void *)cmd);
>       cmd = COMMAND_C;
>       ipc_send_message(chan, (void *)cmd);
> }

This is the example I mentioned above (Make cmd a structure or an
integer array).

> 
> 
>>>>
>>> As I said, among other advantages, TX buffering is needed to reduce
>>> latencies. My platform's protocol isn't yet clear as to whether use
>>> shared-memory (secure and non-secure versions) for not very large data
>>> transfers or interrupt payload. So if we get good enough speed we
>>> could save cost on dedicated memories.
>>>
>>> I wrote the omap2 controller driver code to convey my proposal better.
>>> If you could please tell me how TX buffering is a problem for OMAP, it
>>> will help me understand your concern better.
>>
>> It is not a problem for OMAP
>>
> Thanks for confirming that it's not a problem for OMAP.
> 
> 
>>  The other thing is that you
>> have defined 10 as the queue length in the core, this should be dictated
>> by the controller or mailbox. What works for one platform may not work
>> for some others (this is what you saw as different config options
>> between OMAP and DBX500)
>>
> Please read the multi-line comment over that single line define.
> The buffer is cheap, just a void*, so we could make it even 50.
> However I am not hung on that.

OK good, because no single number would be good for all the controllers
or all possible clients.

> It purely depends upon how the client uses the link,

i guess you are saying this because of the possible blocking behavior of
a client, or the exclusive access to a link.

> so it can't be driven by the controller. We could make it a Kconfig option.
> What do you suggest?

I am saying controller/link because they are the ones that knows how the
physical transport is, and it may vary from one to another. I would
think that the client dependencies would become a subset of this.
Kconfig option is fine, but we have to think about the best way of
representing it from a multi-platform kernel support perspective.

>>>>
>>> How could the core perform any meaningful sanity check?  What if we
>>> send/receive proper 'datalen' of garbage data?  What if the client
>>> sends a "Give me info in not more than 32-bytes at a time" command to
>>> the remote and the remote has valid info worth only 16 bytes?
>>> IMHO there is not much for core to verify in a packet in transit. The
>>> introduced checks are not worth it and actually limit some scenarios
>>> like above example.
>>
>> OK, I had mainly two ideas, one telling the size of the void * on Tx,
>> with the controller driver specifying max (and maybe min sizes expected)
>>
> No, please. The controller driver is supposed to be reusable over
> various protocols. A protocol dictates the length of each logical TX
> unit. The max-min limits set by the controller driver might work for
> one but fail some other higher level protocol.
> 
>>
>> The second (major reason) idea was driven by the kfifo implementation
>> for buffering in core code for which I need the size field, so that the
>> clients need not be intelligent about the status of its tx submission
>> and maintaining its pointer (if you have different calling contexts). I
>> guess both of us are coming from different perspectives here, you may
>> find kfifos inefficient and I may find the complexity in client driver
>> unnecessary for maintaining pointers. How about defining add and remove
>> buffer ops in the controller driver so that it can choose its
>> implementation, while we keep the overall flow architecture in the core
>> code?
>>
> As I said the API shouldn't keep internal copies of a client's
> requests - it will only kill zero-copy use-cases while adding
> complexity to the core.

This is the reason I was suggesting the nature be dictated by a
controller, rather than 1 design in core code mandating a certain usage.
The client & controller together would know the functional integration
aspect.

> Since it already works fine for OMAP, I request we leave it as such
> for now at least.

OK, we should make a note of it in some documentation.

>>>>
>>>> Yeah, assumptions hold true until an unfortunate SoC comes up with it
>>>> :). The TI AM3352 has one link that doesn't behave quite like the other
>>>> (I am hoping to avoid using it), so I am ok with it for now. Guess we
>>>> can always change it when a concrete need arises.
>>>>
>>> It would work right now :)   If your controller has 2 types of links,
>>> the driver should register each bunch separately, each with its own
>>> ipc_link_ops, with the core. The core can already take care of the
>>> rest.
>>
>> Yeah, it can be done that way, but that's a not-so-elegant work-around
>>
> OTOH I find it elegant and not a "work around".
> Liberally estimating, hardly 10% of IPC controllers would have 2
> classes of physical links. Having link_ops per controller reduces the
> number of ipc_link_ops structures 11:20 as compared to per link.
> 
> 
>> IMHO. It's calling ipc_links_register twice on the same controller. Idea
>> was that the API would distinguish different controllers, not the same
>> one. Also, see the above example if you were to treat a link as Rx or Tx
>> only (functional behavior differences even though the link is exactly
>> the same type).
>>
> Why is calling ipc_links_register() twice, for a controller with 2
> classes of links, a problem ?

It will be inelegant, once you start maintaining the list of
ipc_controllers in the core code. You would have to search all the
controllers in the list with the matching name, and their links. You
will need to define multiple controller specific controller structures
and copy most of the elements from the actual h/w device into the
containing ipc_controller definition. As you said, the clients deal with
the links themselves, so making the ops part of the ipc_link definition
makes it easier on the controller implementations. You are anyway
caching the ops in ipc_chan (per link) and that is what you are using in
the core code. Majority of the time, you would be using the same ops for
all the links, but this gives the flexibility to links. You can avoid
having multiple controller instance denoting the h/w block, just because
of the difference in links behavior.

> 
> And, No, the API need not know if the link is "RX" or "TX", which is
> purely a logical function.
> 
> Please do tell me which controller physically limits its links to do
> only "RX" or only "TX"?  It's just the platform's firmware that
> dictates who sends and who receives on which link, thereby giving the
> illusion of a link being a "RX" or "TX" capable only.
> Even if some h/w limits do exist on links of some controller, still
> the API shouldn't try to discern a "RX" from a "TX" link. When a
> client knows which link to open, it will also know if it should read
> or write to it - this assumption already holds true. If the client
> writes on a link that it's supposed to listen to remote on, no amount
> of policing by the API could help.
>
> Even if the API tries to differentiate "RX" and "TX" links, the
> knowledge must come from a client (it's driven by the protocol on the
> platform), and not the controller because each of its links are
> physically equally capable to do RX or TX on each end.

The API never differentiates, but the controller implementation has to.
>From a controller driver implementation perspective, you still have to
make sure that no client is calling an op on which it is not supposed
to. When you have a mixture like this, a common code would include some
dead paths for certain links.

> 
> 
>>> Yes, I have 2 types of controllers and I already thought about
>>> multiple controllers in a platform.
>>>  What do you expect to do in controller startup/shutdown? Since the
>>> API works on individual links, not the controllers, when would it call
>>> controller_start() and controller_shutdown()?  The ipc_link's
>>> startup/shutdown are already non-atomic, if the controller needs any
>>> setup it could do it in the first link's startup and teardown in the
>>> last link's shutdown. Not to forget the resources lke IRQ are usually
>>> per Link which should be acquired/released upon link's use/idle.
>>>  While writing the OMAP2 controller driver, did I miss any controller
>>> startup/shutdown ?
>>
>> It just delineates the code better, and has flexibility later on to
>> extend any controller ops or exposing new controller-specific API. The
>> controller start and stop will be called in the same function as
>> ipc_request_channel.
>>
> 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.

> 
> Why have 2 contiguous calls to the controller, to perform similar
> functions?  Not to forget the API and clients don't work on IPC
> controllers, but only on links. So it would be out-of-line for them to
> have to worry about the controller.

As I said, mainly to help dilineate the controller implementation code
better. I can live with it not being present, as my startup function
would anyway be refactored into two - one for the controller, and one
for the link itself. And I bet most would do the same if they have
multiple links in a controller.

> 
>>
>> 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. Anyway, this is
just an FYI, as this file will obviously be different since I have to
not break my existing clients.

regards
Suman

[RFC 2/3] mailbox: Introduce a new common API

[Jassi Brar]

Hi Suman,

On 9 May 2013 06:55, Suman Anna <s-anna@ti.com> wrote:

>> so it can't be driven by the controller. We could make it a Kconfig option.
>> What do you suggest?
>
> I am saying controller/link because they are the ones that knows how the
> physical transport is, and it may vary from one to another. I would
> think that the client dependencies would become a subset of this.
> Kconfig option is fine, but we have to think about the best way of
> representing it from a multi-platform kernel support perspective.
>
Actually it has more to do with clients and less with multiplatform.
Even on single platform the client that needs biggest buffer will
rule. Ditto on multi-platform.
Anyways I get your idea. I'll put a note there to revisit once number
of active clients increase on the platform.


>> As I said the API shouldn't keep internal copies of a client's
>> requests - it will only kill zero-copy use-cases while adding
>> complexity to the core.
>
> This is the reason I was suggesting the nature be dictated by a
> controller, rather than 1 design in core code mandating a certain usage.
> The client & controller together would know the functional integration
> aspect.
>
>> Since it already works fine for OMAP, I request we leave it as such
>> for now at least.
>
> OK, we should make a note of it in some documentation.
>
OK, will do. Thanks.


>>> IMHO. It's calling ipc_links_register twice on the same controller. Idea
>>> was that the API would distinguish different controllers, not the same
>>> one. Also, see the above example if you were to treat a link as Rx or Tx
>>> only (functional behavior differences even though the link is exactly
>>> the same type).
>>>
>> Why is calling ipc_links_register() twice, for a controller with 2
>> classes of links, a problem ?
>
> It will be inelegant, once you start maintaining the list of
> ipc_controllers in the core code. You would have to search all the
> controllers in the list with the matching name, and their links. You
> will need to define multiple controller specific controller structures
> and copy most of the elements from the actual h/w device into the
> containing ipc_controller definition. As you said, the clients deal with
> the links themselves, so making the ops part of the ipc_link definition
> makes it easier on the controller implementations. You are anyway
> caching the ops in ipc_chan (per link) and that is what you are using in
> the core code. Majority of the time, you would be using the same ops for
> all the links, but this gives the flexibility to links. You can avoid
> having multiple controller instance denoting the h/w block, just because
> of the difference in links behavior.
>
I don't see it as an issue, it's just how the code is designed on the
principle that the API works only on links, not controllers. Let us
wait for the driver of such a controller to show up. It should be easy
to change if we see the need.


>>
>> And, No, the API need not know if the link is "RX" or "TX", which is
>> purely a logical function.
>>
>> Please do tell me which controller physically limits its links to do
>> only "RX" or only "TX"?  It's just the platform's firmware that
>> dictates who sends and who receives on which link, thereby giving the
>> illusion of a link being a "RX" or "TX" capable only.
>> Even if some h/w limits do exist on links of some controller, still
>> the API shouldn't try to discern a "RX" from a "TX" link. When a
>> client knows which link to open, it will also know if it should read
>> or write to it - this assumption already holds true. If the client
>> writes on a link that it's supposed to listen to remote on, no amount
>> of policing by the API could help.
>>
>> Even if the API tries to differentiate "RX" and "TX" links, the
>> knowledge must come from a client (it's driven by the protocol on the
>> platform), and not the controller because each of its links are
>> physically equally capable to do RX or TX on each end.
>
> The API never differentiates, but the controller implementation has to.
> From a controller driver implementation perspective, you still have to
> make sure that no client is calling an op on which it is not supposed
> to. When you have a mixture like this, a common code would include some
> dead paths for certain links.
>
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.


>>>> Yes, I have 2 types of controllers and I already thought about
>>>> multiple controllers in a platform.
>>>>  What do you expect to do in controller startup/shutdown? Since the
>>>> API works on individual links, not the controllers, when would it call
>>>> controller_start() and controller_shutdown()?  The ipc_link's
>>>> startup/shutdown are already non-atomic, if the controller needs any
>>>> setup it could do it in the first link's startup and teardown in the
>>>> last link's shutdown. Not to forget the resources lke IRQ are usually
>>>> per Link which should be acquired/released upon link's use/idle.
>>>>  While writing the OMAP2 controller driver, did I miss any controller
>>>> startup/shutdown ?
>>>
>>> It just delineates the code better, and has flexibility later on to
>>> extend any controller ops or exposing new controller-specific API. The
>>> controller start and stop will be called in the same function as
>>> ipc_request_channel.
>>>
>> 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?  If no, then the ipc_con_ops.startup() will be
really executed only upon probe or resume, which is controller
driver's internal matter.

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?


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

Regards,
-Jassi

[RFC 2/3] mailbox: Introduce a new common API

[Suman Anna]

Hi Jassi,
>
> On 9 May 2013 06:55, Suman Anna <s-anna@ti.com> wrote:
> 
>>> so it can't be driven by the controller. We could make it a Kconfig option.
>>> What do you suggest?
>>
>> I am saying controller/link because they are the ones that knows how the
>> physical transport is, and it may vary from one to another. I would
>> think that the client dependencies would become a subset of this.
>> Kconfig option is fine, but we have to think about the best way of
>> representing it from a multi-platform kernel support perspective.
>>
> Actually it has more to do with clients and less with multiplatform.
> Even on single platform the client that needs biggest buffer will
> rule. Ditto on multi-platform.
> Anyways I get your idea. I'll put a note there to revisit once number
> of active clients increase on the platform.

OK thanks.

>>>> IMHO. It's calling ipc_links_register twice on the same controller. Idea
>>>> was that the API would distinguish different controllers, not the same
>>>> one. Also, see the above example if you were to treat a link as Rx or Tx
>>>> only (functional behavior differences even though the link is exactly
>>>> the same type).
>>>>
>>> Why is calling ipc_links_register() twice, for a controller with 2
>>> classes of links, a problem ?
>>
>> It will be inelegant, once you start maintaining the list of
>> ipc_controllers in the core code. You would have to search all the
>> controllers in the list with the matching name, and their links. You
>> will need to define multiple controller specific controller structures
>> and copy most of the elements from the actual h/w device into the
>> containing ipc_controller definition. As you said, the clients deal with
>> the links themselves, so making the ops part of the ipc_link definition
>> makes it easier on the controller implementations. You are anyway
>> caching the ops in ipc_chan (per link) and that is what you are using in
>> the core code. Majority of the time, you would be using the same ops for
>> all the links, but this gives the flexibility to links. You can avoid
>> having multiple controller instance denoting the h/w block, just because
>> of the difference in links behavior.
>>
> I don't see it as an issue, it's just how the code is designed on the
> principle that the API works only on links, not controllers. Let us
> wait for the driver of such a controller to show up. It should be easy
> to change if we see the need.

OK fine, I brought it up since we are defining the API, and defining it
the first time with flexibility eliminates the need later on.

> 
>>>
>>> And, No, the API need not know if the link is "RX" or "TX", which is
>>> purely a logical function.
>>>
>>> Please do tell me which controller physically limits its links to do
>>> only "RX" or only "TX"?  It's just the platform's firmware that
>>> dictates who sends and who receives on which link, thereby giving the
>>> illusion of a link being a "RX" or "TX" capable only.
>>> Even if some h/w limits do exist on links of some controller, still
>>> the API shouldn't try to discern a "RX" from a "TX" link. When a
>>> client knows which link to open, it will also know if it should read
>>> or write to it - this assumption already holds true. If the client
>>> writes on a link that it's supposed to listen to remote on, no amount
>>> of policing by the API could help.
>>>
>>> Even if the API tries to differentiate "RX" and "TX" links, the
>>> knowledge must come from a client (it's driven by the protocol on the
>>> platform), and not the controller because each of its links are
>>> physically equally capable to do RX or TX on each end.
>>
>> The API never differentiates, but the controller implementation has to.
>> From a controller driver implementation perspective, you still have to
>> make sure that no client is calling an op on which it is not supposed
>> to. When you have a mixture like this, a common code would include some
>> dead paths for certain links.
>>
> 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. The controller driver has to take
care of what it ought to check for in the startup.

> 
>>>>> Yes, I have 2 types of controllers and I already thought about
>>>>> multiple controllers in a platform.
>>>>>  What do you expect to do in controller startup/shutdown? Since the
>>>>> API works on individual links, not the controllers, when would it call
>>>>> controller_start() and controller_shutdown()?  The ipc_link's
>>>>> startup/shutdown are already non-atomic, if the controller needs any
>>>>> setup it could do it in the first link's startup and teardown in the
>>>>> last link's shutdown. Not to forget the resources lke IRQ are usually
>>>>> per Link which should be acquired/released upon link's use/idle.
>>>>>  While writing the OMAP2 controller driver, did I miss any controller
>>>>> startup/shutdown ?
>>>>
>>>> It just delineates the code better, and has flexibility later on to
>>>> extend any controller ops or exposing new controller-specific API. The
>>>> controller start and stop will be called in the same function as
>>>> ipc_request_channel.
>>>>
>>> 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.

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

> 
>>>> 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)?

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

regards
Suman

[RFC 2/3] mailbox: Introduce a new common API

[Jassi Brar]

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

[RFC 2/3] mailbox: Introduce a new common API

[Suman Anna]

Jassi,

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

You might be missing the minor detail. The pm_runtime_enable is in
startup, which is called for every link. So, I get an unbalanced
warning, when I have
	ipc_request_channel(client1);  -> link1.startup
	ipc_request_channel(clinet2);  -> link2.startup

Anyway, it is a non-issue since I understand the context and the fix.

regards
Suman