Hi Ohad/Hiroshi,
Good to see the new mailbox requirements. While we are at it can we add some
more :)?
* Make the mailbox as a generic driver.
Meaning, users can request for a pair of mailbox rather than using a
set of pre-defined ones. For eg: Instead of doing an omap_mbox_get("mailbox-1")
we can have the user specify the mailbox pair that he needs ( omap_mbox_get(int
tx_mbox, int rx_mbox) or similar API ). This also means that we remove the bulk
of the data structures like omap2_mbox_1_priv and mbox_1_info and so on.
Additional checks needs to be done so that consequtive requests to the driver
does not re-configure the rx-tx pairs.
Rationale:
The pre-configured structures does make sense in case of DSP Bridge, since it
is the only user of mailbox. However, for Syslink for example, (or for any
other IPC or user of mailbox) it would be good for the user of the mailbox to
request the pair (or just tx/rx) from user/kernel side.
* Provide debug support for the mailboxes (relevant for OMAP4)
Since OMAP4 has support to read the RAW registers we might as well add
an API for the user to read the status from RAW registers.
Please provide your feedback!
Thank you and Regards
Subbu
> -----Original Message-----
> From: linux-omap-ow...@vger.kernel.org
> [mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of Hiroshi DOYU
> Sent: Monday, June 14, 2010 3:58 AM
> To: Guzman Lugo, Fernando; o...@wizery.com
> Cc: Chitriki Rudramuni, Deepak; Ramirez Luna, Omar; Kanigeri,
> Hari; linux-omap@vger.kernel.org
> Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
>
> Hi Ohad,
>
> From: ext Ohad Ben-Cohen <o...@wizery.com>
> Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
> Date: Mon, 14 Jun 2010 01:52:16 +0200
>
> > On Wed, Jun 9, 2010 at 12:07 AM, Hiroshi DOYU
> <hiroshi.d...@nokia.com> wrote:
> >> diff --git a/arch/arm/plat-omap/mailbox.c
> b/arch/arm/plat-omap/mailbox.c
> >> index 87e0cde..1b79b32 100644
> >> --- a/arch/arm/plat-omap/mailbox.c
> >> +++ b/arch/arm/plat-omap/mailbox.c
> >> @@ -188,7 +188,7 @@ static void __mbox_rx_interrupt(struct
> omap_mbox *mbox)
> >> /* no more messages in the fifo. clear IRQ source. */
> >> ack_mbox_irq(mbox, IRQ_RX);
> >> nomem:
> >> - queue_work(mboxd, &mbox->rxq->work);
> >> + mbox->callback(mbox);
> >> }
> >
> > I like this !
> >
> > It will allow us to easily plug in new IPC mechanisms in the future.
>
> Agree.
>
> > (btw, if we do this, and also introduce
> omap_mbox_msg_send_sync, do we
> > still need a mailbox queuing mechanism at all ? :)
>
> I think that "queuing"(can be called as S/W fifo/buffer?) is basically
> necessary it can compensate the shortage of H/W fifo slots under high
> load or emergency case. It has only 4 slots. I guess that, DSP usually
> responds quickly and 4 H/W slots may be enough, but it might be
> safer/more robust to avoid the assumption which depends on other
> entity/DSP. From latecy perspective, "s/w fifo" + "tasklet" would be
> enough short.
>
> omap_mbox_msg_send_sync() can handle the case for a special message,
> like PM, which has to respond at the higher priority than the normal
> ones.
>
> > Having said that, this is not going to solve the lockdep warning
> > reported by Deepak - that was caused because of dspbridge's sending
> > context (and not because of the receiving context). To
> eliminate that
>
> Does dspbridge really need its own defered work for sending mailbox
> messages?
>
> For me, the problem here is the unneccesary duplication of tasklet, or
> can be said, the unnecessary use of tasklet for _sending_ mailbox
> messages in dspbridge.
>
> http://marc.info/?l=linux-omap&m=127601655325416&w=2
>
> I thought that bridge_msg_put()/bridge_msg_get() can use omap mbox
> APIs directly, with getting rid of its use of its own defered
> work/tasklet as pointed out in the above link. Fernando?
>
> For recieving, its defered work(tasklet) can be trigered directly in
> the above proposed callback, that callback can triger its own
> workqueue if necessary, then. I think that, for recieving, some PM
> command may has to be sent back immedieately inside of
> tasklet. omap_mbox_msg_send_sync() may handle this case.
>
> What do you think?
>
> > issue, I prefer fixing dspbridge to use work queues rather
> than using
> > spin_lock_bh in omap_mbox_msg_send. Disabling system bottom halves
> > just to send a mbox msg sounds unjustified (unless bridge
> really needs
> > to use tasklets instead of work queues, which I slightly
> doubt). What
> > do you think ?
>
> I think that workqueue is only necessary when it has to sleep,
> otherwise tasklet is prefered. For _sending_ a message inside of
> dspbridge, I haven't found any reasonable reason to use any defered
> work(softirq, tasklet, workqueue) so far.
>
> > Speaking of mailbox I'd like to address some issues that
> are code related:
> >
> > * Let's add mailbox API to set the callback pointer (it
> feels wrong to
> > let users directly manipulate the mbox structure).
> >
> > * We can also safely move the callback field to the main mbox
> > structure, since it has no usage in the TX mq; it's pretty global to
> > the whole mbox.
> >
> > * Let's make sure no one accidentally registers two receivers on the
> > same time (which would result in one overwriting the
> other's callback
> > field).
>
> I'd like to allow mutiple listners in some way, as the flexibility of
> mailbox functionalty.
>
> > * mbox_configured is a global variable and that breaks support of
> > multiple concurrent mailbox instances. Let's move this
> logic into the
> > instance of mbox.
> >
> > * If we make sure there are no two user of the same mailbox instance
> > (see 3rd *), we can eliminate mbox_configured and its locking (not
> > that doesn't mean there can't be two concurrent sending contexts, it
> > just means they are somewhat related, they have a common receiver,
> > which was registered using only a single call to get).
> >
> > Something like this, pls tell me what you think:
>
> For the above 5 proposals, excpet the multiple listeners issue, they
> seems ok.
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
