* [PATCHv3 00/14] drivers: mailbox: framework creation @ 2013-03-13 3:23 Suman Anna 2013-03-21 11:39 ` Linus Walleij ` (2 more replies) 0 siblings, 3 replies; 43+ messages in thread From: Suman Anna @ 2013-03-13 3:23 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-03-13 3:23 [PATCHv3 00/14] drivers: mailbox: framework creation Suman Anna @ 2013-03-21 11:39 ` Linus Walleij 2013-03-21 23:22 ` Stephen Rothwell 2013-04-21 2:40 ` Jassi Brar 2 siblings, 0 replies; 43+ messages in thread From: Linus Walleij @ 2013-03-21 11:39 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-03-13 3:23 [PATCHv3 00/14] drivers: mailbox: framework creation Suman Anna 2013-03-21 11:39 ` Linus Walleij @ 2013-03-21 23:22 ` Stephen Rothwell 2013-03-21 23:37 ` Anna, Suman 2013-04-21 2:40 ` Jassi Brar 2 siblings, 1 reply; 43+ messages in thread From: Stephen Rothwell @ 2013-03-21 23:22 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-03-21 23:22 ` Stephen Rothwell @ 2013-03-21 23:37 ` Anna, Suman 0 siblings, 0 replies; 43+ messages in thread From: Anna, Suman @ 2013-03-21 23:37 UTC (permalink / raw) To: linux-arm-kernel > > > > 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-03-13 3:23 [PATCHv3 00/14] drivers: mailbox: framework creation Suman Anna 2013-03-21 11:39 ` Linus Walleij 2013-03-21 23:22 ` Stephen Rothwell @ 2013-04-21 2:40 ` Jassi Brar 2013-04-22 15:56 ` Anna, Suman 2 siblings, 1 reply; 43+ messages in thread From: Jassi Brar @ 2013-04-21 2:40 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-21 2:40 ` Jassi Brar @ 2013-04-22 15:56 ` Anna, Suman 2013-04-23 4:51 ` Jassi Brar 0 siblings, 1 reply; 43+ messages in thread From: Anna, Suman @ 2013-04-22 15:56 UTC (permalink / raw) To: linux-arm-kernel > > 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-22 15:56 ` Anna, Suman @ 2013-04-23 4:51 ` Jassi Brar 2013-04-23 19:20 ` Anna, Suman 0 siblings, 1 reply; 43+ messages in thread From: Jassi Brar @ 2013-04-23 4:51 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-23 4:51 ` Jassi Brar @ 2013-04-23 19:20 ` Anna, Suman 2013-04-23 23:30 ` Andy Green ` (2 more replies) 0 siblings, 3 replies; 43+ messages in thread From: Anna, Suman @ 2013-04-23 19:20 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-23 19:20 ` Anna, Suman @ 2013-04-23 23:30 ` Andy Green 2013-04-24 4:39 ` Jassi Brar 2013-04-24 7:39 ` [PATCHv3 00/14] drivers: mailbox: framework creation Loic PALLARDY 2 siblings, 0 replies; 43+ messages in thread From: Andy Green @ 2013-04-23 23:30 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-23 19:20 ` Anna, Suman 2013-04-23 23:30 ` Andy Green @ 2013-04-24 4:39 ` Jassi Brar 2013-04-24 8:08 ` Loic PALLARDY 2013-04-24 7:39 ` [PATCHv3 00/14] drivers: mailbox: framework creation Loic PALLARDY 2 siblings, 1 reply; 43+ messages in thread From: Jassi Brar @ 2013-04-24 4:39 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-24 4:39 ` Jassi Brar @ 2013-04-24 8:08 ` Loic PALLARDY 2013-04-24 8:56 ` Jassi Brar 0 siblings, 1 reply; 43+ messages in thread From: Loic PALLARDY @ 2013-04-24 8:08 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-24 8:08 ` Loic PALLARDY @ 2013-04-24 8:56 ` Jassi Brar 2013-04-24 23:16 ` Suman Anna 0 siblings, 1 reply; 43+ messages in thread From: Jassi Brar @ 2013-04-24 8:56 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-24 8:56 ` Jassi Brar @ 2013-04-24 23:16 ` Suman Anna 2013-04-25 5:20 ` Jassi Brar 0 siblings, 1 reply; 43+ messages in thread From: Suman Anna @ 2013-04-24 23:16 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-24 23:16 ` Suman Anna @ 2013-04-25 5:20 ` Jassi Brar 2013-04-25 22:29 ` Suman Anna 0 siblings, 1 reply; 43+ messages in thread From: Jassi Brar @ 2013-04-25 5:20 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-25 5:20 ` Jassi Brar @ 2013-04-25 22:29 ` Suman Anna 2013-04-25 23:51 ` Andy Green 2013-04-26 3:46 ` Jassi Brar 0 siblings, 2 replies; 43+ messages in thread From: Suman Anna @ 2013-04-25 22:29 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-25 22:29 ` Suman Anna @ 2013-04-25 23:51 ` Andy Green 2013-04-26 3:46 ` Jassi Brar 1 sibling, 0 replies; 43+ messages in thread From: Andy Green @ 2013-04-25 23:51 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-25 22:29 ` Suman Anna 2013-04-25 23:51 ` Andy Green @ 2013-04-26 3:46 ` Jassi Brar 2013-04-27 1:04 ` Suman Anna 1 sibling, 1 reply; 43+ messages in thread From: Jassi Brar @ 2013-04-26 3:46 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-26 3:46 ` Jassi Brar @ 2013-04-27 1:04 ` Suman Anna 2013-04-27 1:48 ` Andy Green 2013-04-27 4:51 ` Jassi Brar 0 siblings, 2 replies; 43+ messages in thread From: Suman Anna @ 2013-04-27 1:04 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-27 1:04 ` Suman Anna @ 2013-04-27 1:48 ` Andy Green 2013-04-29 15:32 ` Suman Anna 2013-04-27 4:51 ` Jassi Brar 1 sibling, 1 reply; 43+ messages in thread From: Andy Green @ 2013-04-27 1:48 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-27 1:48 ` Andy Green @ 2013-04-29 15:32 ` Suman Anna 0 siblings, 0 replies; 43+ messages in thread From: Suman Anna @ 2013-04-29 15:32 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-27 1:04 ` Suman Anna 2013-04-27 1:48 ` Andy Green @ 2013-04-27 4:51 ` Jassi Brar 2013-04-27 18:05 ` [PATCH 1/3] mailbox: rename pl320-ipc specific mailbox.h jaswinder.singh at linaro.org ` (3 more replies) 1 sibling, 4 replies; 43+ messages in thread From: Jassi Brar @ 2013-04-27 4:51 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 1/3] mailbox: rename pl320-ipc specific mailbox.h 2013-04-27 4:51 ` Jassi Brar @ 2013-04-27 18:05 ` jaswinder.singh at linaro.org 2013-04-29 12:46 ` Mark Langsdorf 2013-04-29 16:00 ` [PATCHv3 00/14] drivers: mailbox: framework creation Suman Anna ` (2 subsequent siblings) 3 siblings, 1 reply; 43+ messages in thread From: jaswinder.singh at linaro.org @ 2013-04-27 18:05 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 1/3] mailbox: rename pl320-ipc specific mailbox.h 2013-04-27 18:05 ` [PATCH 1/3] mailbox: rename pl320-ipc specific mailbox.h jaswinder.singh at linaro.org @ 2013-04-29 12:46 ` Mark Langsdorf 0 siblings, 0 replies; 43+ messages in thread From: Mark Langsdorf @ 2013-04-29 12:46 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-27 4:51 ` Jassi Brar 2013-04-27 18:05 ` [PATCH 1/3] mailbox: rename pl320-ipc specific mailbox.h jaswinder.singh at linaro.org @ 2013-04-29 16:00 ` Suman Anna 2013-04-29 16:49 ` Jassi Brar [not found] ` <1367086496-28647-1-git-send-email-jaswinder.singh@linaro.org> [not found] ` <1367086474-28614-1-git-send-email-jaswinder.singh@linaro.org> 3 siblings, 1 reply; 43+ messages in thread From: Suman Anna @ 2013-04-29 16:00 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-29 16:00 ` [PATCHv3 00/14] drivers: mailbox: framework creation Suman Anna @ 2013-04-29 16:49 ` Jassi Brar 0 siblings, 0 replies; 43+ messages in thread From: Jassi Brar @ 2013-04-29 16:49 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
[parent not found: <1367086496-28647-1-git-send-email-jaswinder.singh@linaro.org>]
* [RFC 3/3] mailbox: pl320: Introduce common API driver [not found] ` <1367086496-28647-1-git-send-email-jaswinder.singh@linaro.org> @ 2013-04-29 16:44 ` Suman Anna 2013-04-29 16:57 ` Jassi Brar 0 siblings, 1 reply; 43+ messages in thread From: Suman Anna @ 2013-04-29 16:44 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC 3/3] mailbox: pl320: Introduce common API driver 2013-04-29 16:44 ` [RFC 3/3] mailbox: pl320: Introduce common API driver Suman Anna @ 2013-04-29 16:57 ` Jassi Brar 2013-04-29 17:06 ` Mark Langsdorf 0 siblings, 1 reply; 43+ messages in thread From: Jassi Brar @ 2013-04-29 16:57 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC 3/3] mailbox: pl320: Introduce common API driver 2013-04-29 16:57 ` Jassi Brar @ 2013-04-29 17:06 ` Mark Langsdorf 2013-04-29 17:28 ` Jassi Brar 0 siblings, 1 reply; 43+ messages in thread From: Mark Langsdorf @ 2013-04-29 17:06 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC 3/3] mailbox: pl320: Introduce common API driver 2013-04-29 17:06 ` Mark Langsdorf @ 2013-04-29 17:28 ` Jassi Brar 0 siblings, 0 replies; 43+ messages in thread From: Jassi Brar @ 2013-04-29 17:28 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
[parent not found: <1367086474-28614-1-git-send-email-jaswinder.singh@linaro.org>]
* [RFC 2/3] mailbox: Introduce a new common API [not found] ` <1367086474-28614-1-git-send-email-jaswinder.singh@linaro.org> @ 2013-05-04 2:20 ` Suman Anna 2013-05-04 19:08 ` Jassi Brar 0 siblings, 1 reply; 43+ messages in thread From: Suman Anna @ 2013-05-04 2:20 UTC (permalink / raw) To: linux-arm-kernel 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; > +}; > + ^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC 2/3] mailbox: Introduce a new common API 2013-05-04 2:20 ` [RFC 2/3] mailbox: Introduce a new common API Suman Anna @ 2013-05-04 19:08 ` Jassi Brar 2013-05-06 23:45 ` Suman Anna 0 siblings, 1 reply; 43+ messages in thread From: Jassi Brar @ 2013-05-04 19:08 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC 2/3] mailbox: Introduce a new common API 2013-05-04 19:08 ` Jassi Brar @ 2013-05-06 23:45 ` Suman Anna 2013-05-07 7:40 ` Jassi Brar 0 siblings, 1 reply; 43+ messages in thread From: Suman Anna @ 2013-05-06 23:45 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC 2/3] mailbox: Introduce a new common API 2013-05-06 23:45 ` Suman Anna @ 2013-05-07 7:40 ` Jassi Brar 2013-05-07 21:48 ` Suman Anna 0 siblings, 1 reply; 43+ messages in thread From: Jassi Brar @ 2013-05-07 7:40 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC 2/3] mailbox: Introduce a new common API 2013-05-07 7:40 ` Jassi Brar @ 2013-05-07 21:48 ` Suman Anna 2013-05-08 5:44 ` Jassi Brar 0 siblings, 1 reply; 43+ messages in thread From: Suman Anna @ 2013-05-07 21:48 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC 2/3] mailbox: Introduce a new common API 2013-05-07 21:48 ` Suman Anna @ 2013-05-08 5:44 ` Jassi Brar 2013-05-09 1:25 ` Suman Anna 0 siblings, 1 reply; 43+ messages in thread From: Jassi Brar @ 2013-05-08 5:44 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC 2/3] mailbox: Introduce a new common API 2013-05-08 5:44 ` Jassi Brar @ 2013-05-09 1:25 ` Suman Anna 2013-05-09 16:35 ` Jassi Brar 0 siblings, 1 reply; 43+ messages in thread From: Suman Anna @ 2013-05-09 1:25 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC 2/3] mailbox: Introduce a new common API 2013-05-09 1:25 ` Suman Anna @ 2013-05-09 16:35 ` Jassi Brar 2013-05-10 0:18 ` Suman Anna 0 siblings, 1 reply; 43+ messages in thread From: Jassi Brar @ 2013-05-09 16:35 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC 2/3] mailbox: Introduce a new common API 2013-05-09 16:35 ` Jassi Brar @ 2013-05-10 0:18 ` Suman Anna 2013-05-10 10:06 ` Jassi Brar 0 siblings, 1 reply; 43+ messages in thread From: Suman Anna @ 2013-05-10 0:18 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC 2/3] mailbox: Introduce a new common API 2013-05-10 0:18 ` Suman Anna @ 2013-05-10 10:06 ` Jassi Brar 2013-05-10 16:41 ` Suman Anna 0 siblings, 1 reply; 43+ messages in thread From: Jassi Brar @ 2013-05-10 10:06 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC 2/3] mailbox: Introduce a new common API 2013-05-10 10:06 ` Jassi Brar @ 2013-05-10 16:41 ` Suman Anna 0 siblings, 0 replies; 43+ messages in thread From: Suman Anna @ 2013-05-10 16:41 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-23 19:20 ` Anna, Suman 2013-04-23 23:30 ` Andy Green 2013-04-24 4:39 ` Jassi Brar @ 2013-04-24 7:39 ` Loic PALLARDY 2013-04-24 7:59 ` Jassi Brar 2 siblings, 1 reply; 43+ messages in thread From: Loic PALLARDY @ 2013-04-24 7:39 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-24 7:39 ` [PATCHv3 00/14] drivers: mailbox: framework creation Loic PALLARDY @ 2013-04-24 7:59 ` Jassi Brar 2013-04-24 8:39 ` Loic PALLARDY 0 siblings, 1 reply; 43+ messages in thread From: Jassi Brar @ 2013-04-24 7:59 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCHv3 00/14] drivers: mailbox: framework creation 2013-04-24 7:59 ` Jassi Brar @ 2013-04-24 8:39 ` Loic PALLARDY 0 siblings, 0 replies; 43+ messages in thread From: Loic PALLARDY @ 2013-04-24 8:39 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
end of thread, other threads:[~2013-05-10 16:41 UTC | newest] Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-03-13 3:23 [PATCHv3 00/14] drivers: mailbox: framework creation Suman Anna 2013-03-21 11:39 ` Linus Walleij 2013-03-21 23:22 ` Stephen Rothwell 2013-03-21 23:37 ` Anna, Suman 2013-04-21 2:40 ` Jassi Brar 2013-04-22 15:56 ` Anna, Suman 2013-04-23 4:51 ` Jassi Brar 2013-04-23 19:20 ` Anna, Suman 2013-04-23 23:30 ` Andy Green 2013-04-24 4:39 ` Jassi Brar 2013-04-24 8:08 ` Loic PALLARDY 2013-04-24 8:56 ` Jassi Brar 2013-04-24 23:16 ` Suman Anna 2013-04-25 5:20 ` Jassi Brar 2013-04-25 22:29 ` Suman Anna 2013-04-25 23:51 ` Andy Green 2013-04-26 3:46 ` Jassi Brar 2013-04-27 1:04 ` Suman Anna 2013-04-27 1:48 ` Andy Green 2013-04-29 15:32 ` Suman Anna 2013-04-27 4:51 ` Jassi Brar 2013-04-27 18:05 ` [PATCH 1/3] mailbox: rename pl320-ipc specific mailbox.h jaswinder.singh at linaro.org 2013-04-29 12:46 ` Mark Langsdorf 2013-04-29 16:00 ` [PATCHv3 00/14] drivers: mailbox: framework creation Suman Anna 2013-04-29 16:49 ` Jassi Brar [not found] ` <1367086496-28647-1-git-send-email-jaswinder.singh@linaro.org> 2013-04-29 16:44 ` [RFC 3/3] mailbox: pl320: Introduce common API driver Suman Anna 2013-04-29 16:57 ` Jassi Brar 2013-04-29 17:06 ` Mark Langsdorf 2013-04-29 17:28 ` Jassi Brar [not found] ` <1367086474-28614-1-git-send-email-jaswinder.singh@linaro.org> 2013-05-04 2:20 ` [RFC 2/3] mailbox: Introduce a new common API Suman Anna 2013-05-04 19:08 ` Jassi Brar 2013-05-06 23:45 ` Suman Anna 2013-05-07 7:40 ` Jassi Brar 2013-05-07 21:48 ` Suman Anna 2013-05-08 5:44 ` Jassi Brar 2013-05-09 1:25 ` Suman Anna 2013-05-09 16:35 ` Jassi Brar 2013-05-10 0:18 ` Suman Anna 2013-05-10 10:06 ` Jassi Brar 2013-05-10 16:41 ` Suman Anna 2013-04-24 7:39 ` [PATCHv3 00/14] drivers: mailbox: framework creation Loic PALLARDY 2013-04-24 7:59 ` Jassi Brar 2013-04-24 8:39 ` Loic PALLARDY
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).