[PATCH 1/4] omap mailbox: extend API to take a callback param

[PATCH 1/4] omap mailbox: extend API to take a callback param

[Ohad Ben-Cohen]

Let mailbox users set the callback pointer via the
omap_mbox_get API, instead of having users directly
manipulating the mailbox structures.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
I can also be contacted at < ohadb at ti dot com >

 arch/arm/plat-omap/include/plat/mailbox.h |    2 +-
 arch/arm/plat-omap/mailbox.c              |    4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-omap/include/plat/mailbox.h
index 9976565..0b45664 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -62,7 +62,7 @@ struct omap_mbox {
 int omap_mbox_msg_send(struct omap_mbox *, mbox_msg_t msg);
 void omap_mbox_init_seq(struct omap_mbox *);
 
-struct omap_mbox *omap_mbox_get(const char *);
+struct omap_mbox *omap_mbox_get(const char *name, int (*callback)(void *));
 void omap_mbox_put(struct omap_mbox *);
 
 int omap_mbox_register(struct device *parent, struct omap_mbox **);
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index d2fafb8..14b716d 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -305,7 +305,7 @@ static void omap_mbox_fini(struct omap_mbox *mbox)
 	}
 }
 
-struct omap_mbox *omap_mbox_get(const char *name)
+struct omap_mbox *omap_mbox_get(const char *name, int (*callback)(void *))
 {
 	struct omap_mbox *mbox;
 	int ret;
@@ -324,6 +324,8 @@ struct omap_mbox *omap_mbox_get(const char *name)
 	if (ret)
 		return ERR_PTR(-ENODEV);
 
+	mbox->rxq->callback = callback;
+
 	return mbox;
 }
 EXPORT_SYMBOL(omap_mbox_get);
-- 
1.7.0.4

[PATCH 2/4] omap mailbox: prevent multiple concurrent receivers race

[Ohad Ben-Cohen]

We currently maintain only a single mailbox receiver callback.
To prevent multiple receivers race scenarios (in which receivers
will end up overwriting each other's callback pointers), we make
sure that mailbox instances cannot be taken by more than
one receiver at the same time.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
I can also be contacted at < ohadb at ti dot com >

 arch/arm/plat-omap/include/plat/mailbox.h |    1 +
 arch/arm/plat-omap/mailbox.c              |   12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-omap/include/plat/mailbox.h
index 0b45664..5df35b4 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -57,6 +57,7 @@ struct omap_mbox {
 	struct omap_mbox_ops	*ops;
 	struct device		*dev;
 	void			*priv;
+	atomic_t		count;
 };
 
 int omap_mbox_msg_send(struct omap_mbox *, mbox_msg_t msg);
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index 14b716d..aafa63f 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -320,9 +320,17 @@ struct omap_mbox *omap_mbox_get(const char *name, int (*callback)(void *))
 	if (!mbox)
 		return ERR_PTR(-ENOENT);
 
+	if (atomic_inc_return(&mbox->count) > 1) {
+		pr_err("%s: mbox %s already in use\n", __func__, name);
+		atomic_dec(&mbox->count);
+		return ERR_PTR(-EBUSY);
+	}
+
 	ret = omap_mbox_startup(mbox);
-	if (ret)
+	if (ret) {
+		atomic_dec(&mbox->count);
 		return ERR_PTR(-ENODEV);
+	}
 
 	mbox->rxq->callback = callback;
 
@@ -333,6 +341,7 @@ EXPORT_SYMBOL(omap_mbox_get);
 void omap_mbox_put(struct omap_mbox *mbox)
 {
 	omap_mbox_fini(mbox);
+	atomic_dec(&mbox->count);
 }
 EXPORT_SYMBOL(omap_mbox_put);
 
@@ -349,6 +358,7 @@ int omap_mbox_register(struct device *parent, struct omap_mbox **list)
 
 	for (i = 0; mboxes[i]; i++) {
 		struct omap_mbox *mbox = mboxes[i];
+		atomic_set(&mbox->count, 0);
 		mbox->dev = device_create(&omap_mbox_class,
 				parent, 0, mbox, "%s", mbox->name);
 		if (IS_ERR(mbox->dev)) {
-- 
1.7.0.4

[PATCH 3/4] omap mailbox: remove mbox_configured scheme

[Ohad Ben-Cohen]

mbox_configured is global and therefore does not support
concurrent usage of multiple mailbox instances.
Instead of fixing this, we can just eliminate mbox_configured
(and its locking) entirely, since any given mailbox instance
can only be taken by a single receiver now.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
I can also be contacted at < ohadb at ti dot com >

 arch/arm/plat-omap/mailbox.c |   24 ++++--------------------
 1 files changed, 4 insertions(+), 20 deletions(-)

diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index aafa63f..6a9dfe5 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -35,9 +35,6 @@ static struct workqueue_struct *mboxd;
 static struct omap_mbox **mboxes;
 static bool rq_full;
 
-static int mbox_configured;
-static DEFINE_MUTEX(mbox_configured_lock);
-
 static unsigned int mbox_kfifo_size = CONFIG_OMAP_MBOX_KFIFO_SIZE;
 module_param(mbox_kfifo_size, uint, S_IRUGO);
 MODULE_PARM_DESC(mbox_kfifo_size, "Size of omap's mailbox kfifo (bytes)");
@@ -240,16 +237,9 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
 	struct omap_mbox_queue *mq;
 
 	if (mbox->ops->startup) {
-		mutex_lock(&mbox_configured_lock);
-		if (!mbox_configured)
-			ret = mbox->ops->startup(mbox);
-
-		if (ret) {
-			mutex_unlock(&mbox_configured_lock);
+		ret = mbox->ops->startup(mbox);
+		if (ret)
 			return ret;
-		}
-		mbox_configured++;
-		mutex_unlock(&mbox_configured_lock);
 	}
 
 	ret = request_irq(mbox->irq, mbox_interrupt, IRQF_SHARED,
@@ -295,14 +285,8 @@ static void omap_mbox_fini(struct omap_mbox *mbox)
 	mbox_queue_free(mbox->txq);
 	mbox_queue_free(mbox->rxq);
 
-	if (mbox->ops->shutdown) {
-		mutex_lock(&mbox_configured_lock);
-		if (mbox_configured > 0)
-			mbox_configured--;
-		if (!mbox_configured)
-			mbox->ops->shutdown(mbox);
-		mutex_unlock(&mbox_configured_lock);
-	}
+	if (mbox->ops->shutdown)
+		mbox->ops->shutdown(mbox);
 }
 
 struct omap_mbox *omap_mbox_get(const char *name, int (*callback)(void *))
-- 
1.7.0.4

[PATCH 4/4] dspbridge: use mailbox API to set rx callback

[Ohad Ben-Cohen]

use mailbox API to set rx callback

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
I can also be contacted at < ohadb at ti dot com >

 drivers/dsp/bridge/core/tiomap3430.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/dsp/bridge/core/tiomap3430.c b/drivers/dsp/bridge/core/tiomap3430.c
index 35c6678..1190c79 100644
--- a/drivers/dsp/bridge/core/tiomap3430.c
+++ b/drivers/dsp/bridge/core/tiomap3430.c
@@ -555,7 +555,7 @@ static int bridge_brd_start(struct bridge_dev_context *hDevContext,
 		 * Enable Mailbox events and also drain any pending
 		 * stale messages.
 		 */
-		dev_context->mbox = omap_mbox_get("dsp");
+		dev_context->mbox = omap_mbox_get("dsp", (int (*)(void *))io_mbox_msg);
 		if (IS_ERR(dev_context->mbox)) {
 			dev_context->mbox = NULL;
 			pr_err("%s: Failed to get dsp mailbox handle\n",
@@ -565,8 +565,6 @@ static int bridge_brd_start(struct bridge_dev_context *hDevContext,
 
 	}
 	if (DSP_SUCCEEDED(status)) {
-		dev_context->mbox->rxq->callback = (int (*)(void *))io_mbox_msg;
-
 /*PM_IVA2GRPSEL_PER = 0xC0;*/
 		temp = (u32) *((reg_uword32 *)
 				((u32) (resources->dw_per_pm_base) + 0xA8));
-- 
1.7.0.4

RE: [PATCH 1/4] omap mailbox: extend API to take a callback param

[Kanigeri, Hari]

Looks good.

Iommu get has similar signature, probably we could change that too to take the mmu fault notification function as a function argument.

Thank you,
Best regards,
Hari

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Ohad Ben-Cohen
> Sent: Tuesday, June 22, 2010 7:12 PM
> To: linux-omap@vger.kernel.org
> Cc: Hiroshi Doyu; Ramirez Luna, Omar; Ohad Ben-Cohen
> Subject: [PATCH 1/4] omap mailbox: extend API to take a callback param
> 
> Let mailbox users set the callback pointer via the
> omap_mbox_get API, instead of having users directly
> manipulating the mailbox structures.
> 
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> ---
> I can also be contacted at < ohadb at ti dot com >
> 
>  arch/arm/plat-omap/include/plat/mailbox.h |    2 +-
>  arch/arm/plat-omap/mailbox.c              |    4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-
> omap/include/plat/mailbox.h
> index 9976565..0b45664 100644
> --- a/arch/arm/plat-omap/include/plat/mailbox.h
> +++ b/arch/arm/plat-omap/include/plat/mailbox.h
> @@ -62,7 +62,7 @@ struct omap_mbox {
>  int omap_mbox_msg_send(struct omap_mbox *, mbox_msg_t msg);
>  void omap_mbox_init_seq(struct omap_mbox *);
> 
> -struct omap_mbox *omap_mbox_get(const char *);
> +struct omap_mbox *omap_mbox_get(const char *name, int (*callback)(void
> *));
>  void omap_mbox_put(struct omap_mbox *);
> 
>  int omap_mbox_register(struct device *parent, struct omap_mbox **);
> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
> index d2fafb8..14b716d 100644
> --- a/arch/arm/plat-omap/mailbox.c
> +++ b/arch/arm/plat-omap/mailbox.c
> @@ -305,7 +305,7 @@ static void omap_mbox_fini(struct omap_mbox *mbox)
>  	}
>  }
> 
> -struct omap_mbox *omap_mbox_get(const char *name)
> +struct omap_mbox *omap_mbox_get(const char *name, int (*callback)(void
> *))
>  {
>  	struct omap_mbox *mbox;
>  	int ret;
> @@ -324,6 +324,8 @@ struct omap_mbox *omap_mbox_get(const char *name)
>  	if (ret)
>  		return ERR_PTR(-ENODEV);
> 
> +	mbox->rxq->callback = callback;
> +
>  	return mbox;
>  }
>  EXPORT_SYMBOL(omap_mbox_get);
> --
> 1.7.0.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/4] omap mailbox: remove mbox_configured scheme

[Kanigeri, Hari]

Ohad,

> mbox_configured is global and therefore does not support
> concurrent usage of multiple mailbox instances.

-- Why do you say that it doesn't support concurrent usage of multiple mailbox instances ? If you take example of OMAP4, we have 2 mailbox instances, one talking to DSP and other to Ducati and they should be supported concurrently.

If you remove the mbox_configured variable, then the mailbox module would shut down once the first instance calls the omap_mbox_put function.

Thank you,
Best regards,
Hari

Re: [PATCH 3/4] omap mailbox: remove mbox_configured scheme

[Ohad Ben-Cohen]

On Thu, Jun 24, 2010 at 6:10 PM, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
> Ohad,
>
>> mbox_configured is global and therefore does not support
>> concurrent usage of multiple mailbox instances.
>
> -- Why do you say that it doesn't support concurrent usage of multiple mailbox instances ? If you take example of OMAP4, we have 2 mailbox instances, one talking to DSP and other to Ducati and they should be supported concurrently.


Interesting. was this actually tested and found working ?

AFAICT (I don't have OMAP4, this is just by looking at the code) this
doesn't work:
mbox_configured will prevent omap2_mbox_startup from being invoked
more than once. This means that omap2_mbox_enable_irq(mbox, IRQ_RX)
will only be called once too, which seems like a problem if you want
to start receiving RX interrupts from both mbox instances at the same
time.

To fix that I guess we should decouple the RX interrupt enabling from
the mbox startup function. I can write something, but I'd need your
help to test it on OMAP4 :)

>
> If you remove the mbox_configured variable, then the mailbox module would shut down
> once the first instance calls the omap_mbox_put function.

Also interesting point.

So you use mbox_configures as a reference counter to allow concurrent
multiple senders to the same mbox instance.

I will update the previous patch (that introduced the atomic_t
reference counter) to reflect this use case, thanks!

>
> Thank you,
> Best regards,
> Hari
>

Re: [PATCH 1/4] omap mailbox: extend API to take a callback param

[Ohad Ben-Cohen]

>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] On Behalf Of Ohad Ben-Cohen
>> Sent: Tuesday, June 22, 2010 7:12 PM
>> To: linux-omap@vger.kernel.org
>> Cc: Hiroshi Doyu; Ramirez Luna, Omar; Ohad Ben-Cohen
>> Subject: [PATCH 1/4] omap mailbox: extend API to take a callback param
>>
>> Let mailbox users set the callback pointer via the
>> omap_mbox_get API, instead of having users directly
>> manipulating the mailbox structures.


On Thu, Jun 24, 2010 at 6:02 PM, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
> Looks good.

PCMIIW: from your other mail, I now understand that on OMAP4 you call
omap_mbox_get several times to allow several concurrent mbox senders.

If that's the case, this new API concept is a bit broken: it should
allow several senders to call it, but will only support a single rx
callback (probably by ignoring NULL callbacks, but that's not really
safe).

We have two options here:

1. do not extend omap_mbox_get to get a callback. instead, add a new
API that explicitely sets the callback pointer.

2. do extend omap_mbox_get to get a callback, but then employ
something like a notifier chain of callbacks, essentially supporting
multiple callbacks. this will allow a more flexible API, something
that Hiroshi also wanted.

I like (2) better.

What do you think ?

Thanks,
Ohad.

RE: [PATCH 3/4] omap mailbox: remove mbox_configured scheme

[C.A, Subramaniam]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Ohad Ben-Cohen
> Sent: Thursday, June 24, 2010 3:23 PM
> To: Kanigeri, Hari
> Cc: linux-omap@vger.kernel.org; Hiroshi Doyu; Ramirez Luna, Omar
> Subject: Re: [PATCH 3/4] omap mailbox: remove mbox_configured scheme
> 
> On Thu, Jun 24, 2010 at 6:10 PM, Kanigeri, Hari 
> <h-kanigeri2@ti.com> wrote:
> > Ohad,
> >
> >> mbox_configured is global and therefore does not support 
> concurrent 
> >> usage of multiple mailbox instances.
> >
> > -- Why do you say that it doesn't support concurrent usage 
> of multiple mailbox instances ? If you take example of OMAP4, 
> we have 2 mailbox instances, one talking to DSP and other to 
> Ducati and they should be supported concurrently.
> 
> 
> Interesting. was this actually tested and found working ?
> 
I think by supporting multiple instances, what Hari meant was that, for DSP and Ducati 2 different struct omap_mbox * are returned. They still can maintain their own callback function 
(http://dev.omapzoom.org/?p=tisyslink/kernel-syslink.git;a=blob;f=arch/arm/mach-omap2/mailbox.c;h=a23c7edf1e84ab4fa51e56d0c2daf2922084751a;hb=438f7a6a3cced478eb121426201206f6205fbbdc#l327 for Ducati
and

http://dev.omapzoom.org/?p=tisyslink/kernel-syslink.git;a=blob;f=arch/arm/mach-omap2/mailbox.c;h=a23c7edf1e84ab4fa51e56d0c2daf2922084751a;hb=438f7a6a3cced478eb121426201206f6205fbbdc#l334 
for DSP).

The "mbox_configured" was a means to reference count for one instance of the struct omap_mbox * .
  
> AFAICT (I don't have OMAP4, this is just by looking at the 
> code) this doesn't work:
> mbox_configured will prevent omap2_mbox_startup from being 
> invoked more than once. This means that 
> omap2_mbox_enable_irq(mbox, IRQ_RX) will only be called once 
> too, which seems like a problem if you want to start 
> receiving RX interrupts from both mbox instances at the same time.
> 
> To fix that I guess we should decouple the RX interrupt 
> enabling from the mbox startup function. I can write 
> something, but I'd need your help to test it on OMAP4 :)
> 
> >
> > If you remove the mbox_configured variable, then the mailbox module 
> > would shut down once the first instance calls the 
> omap_mbox_put function.
> 
> Also interesting point.
> 
> So you use mbox_configures as a reference counter to allow 
> concurrent multiple senders to the same mbox instance.
> 
> I will update the previous patch (that introduced the 
> atomic_t reference counter) to reflect this use case, thanks!
> 
> >
> > Thank you,
> > Best regards,
> > Hari
> >
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-omap" in the body of a message to 
> majordomo@vger.kernel.org More majordomo info at  
> http://vger.kernel.org/majordomo-info.html
> 


Thank you and Regards
Subbu

RE: [PATCH 3/4] omap mailbox: remove mbox_configured scheme

[C.A, Subramaniam]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of C.A, 
> Subramaniam
> Sent: Thursday, June 24, 2010 4:36 PM
> To: Ohad Ben-Cohen; Kanigeri, Hari
> Cc: linux-omap@vger.kernel.org; Hiroshi Doyu; Ramirez Luna, Omar
> Subject: RE: [PATCH 3/4] omap mailbox: remove mbox_configured scheme
> 
> 
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org 
> > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Ohad 
> Ben-Cohen
> > Sent: Thursday, June 24, 2010 3:23 PM
> > To: Kanigeri, Hari
> > Cc: linux-omap@vger.kernel.org; Hiroshi Doyu; Ramirez Luna, Omar
> > Subject: Re: [PATCH 3/4] omap mailbox: remove mbox_configured scheme
> > 
> > On Thu, Jun 24, 2010 at 6:10 PM, Kanigeri, Hari 
> <h-kanigeri2@ti.com> 
> > wrote:
> > > Ohad,
> > >
> > >> mbox_configured is global and therefore does not support
> > concurrent
> > >> usage of multiple mailbox instances.
> > >
> > > -- Why do you say that it doesn't support concurrent usage
> > of multiple mailbox instances ? If you take example of 
> OMAP4, we have 
> > 2 mailbox instances, one talking to DSP and other to Ducati 
> and they 
> > should be supported concurrently.
> > 
> > 
> > Interesting. was this actually tested and found working ?
> > 
> I think by supporting multiple instances, what Hari meant was 
> that, for DSP and Ducati 2 different struct omap_mbox * are 
> returned. They still can maintain their own callback function
> (http://dev.omapzoom.org/?p=tisyslink/kernel-syslink.git;a=blo
> b;f=arch/arm/mach-omap2/mailbox.c;h=a23c7edf1e84ab4fa51e56d0c2
> daf2922084751a;hb=438f7a6a3cced478eb121426201206f6205fbbdc#l32
> 7 for Ducati and
> 
> http://dev.omapzoom.org/?p=tisyslink/kernel-syslink.git;a=blob
> ;f=arch/arm/mach-omap2/mailbox.c;h=a23c7edf1e84ab4fa51e56d0c2d
> af2922084751a;hb=438f7a6a3cced478eb121426201206f6205fbbdc#l334
> for DSP).
> 
> The "mbox_configured" was a means to reference count for one 
> instance of the struct omap_mbox * .
Sorry I that that back. Even though we maintain 2 strucutres, only the one that calls the request_irq() is honoured. Hence yes only one callback will be serviced (Thank to Ramesh for pointing that out). Chaining of callbacks is a good way to allow multiple clients to be notified.

>   
> > AFAICT (I don't have OMAP4, this is just by looking at the
> > code) this doesn't work:
> > mbox_configured will prevent omap2_mbox_startup from being invoked 
> > more than once. This means that omap2_mbox_enable_irq(mbox, IRQ_RX) 
> > will only be called once too, which seems like a problem if 
> you want 
> > to start receiving RX interrupts from both mbox instances 
> at the same 
> > time.
> > 
> > To fix that I guess we should decouple the RX interrupt 
> enabling from 
> > the mbox startup function. I can write something, but I'd need your 
> > help to test it on OMAP4 :)
> > 
> > >
> > > If you remove the mbox_configured variable, then the 
> mailbox module 
> > > would shut down once the first instance calls the
> > omap_mbox_put function.
> > 
> > Also interesting point.
> > 
> > So you use mbox_configures as a reference counter to allow 
> concurrent 
> > multiple senders to the same mbox instance.
> > 
> > I will update the previous patch (that introduced the atomic_t 
> > reference counter) to reflect this use case, thanks!
> > 
> > >
> > > Thank you,
> > > Best regards,
> > > Hari
> > >
> > --
> > To unsubscribe from this list: send the line "unsubscribe 
> linux-omap" 
> > in the body of a message to majordomo@vger.kernel.org More 
> majordomo 
> > info at http://vger.kernel.org/majordomo-info.html
> > 
> 
> 
> Thank you and Regards
> Subbu--
> To unsubscribe from this list: send the line "unsubscribe 
> linux-omap" in the body of a message to 
> majordomo@vger.kernel.org More majordomo info at  
> http://vger.kernel.org/majordomo-info.html
> 

Thank you and Regards
Subbu

RE: [PATCH 3/4] omap mailbox: remove mbox_configured scheme

[Gupta, Ramesh]

Hi Ohad,


> -----Original Message-----
> From: C.A, Subramaniam 
> Sent: Thursday, June 24, 2010 5:24 PM
> To: C.A, Subramaniam; Ohad Ben-Cohen; Kanigeri, Hari; Gupta, Ramesh
> Cc: linux-omap@vger.kernel.org; Hiroshi Doyu; Ramirez Luna, Omar
> Subject: RE: [PATCH 3/4] omap mailbox: remove mbox_configured scheme
> 
> 
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org 
> > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of C.A, 
> > Subramaniam
> > Sent: Thursday, June 24, 2010 4:36 PM
> > To: Ohad Ben-Cohen; Kanigeri, Hari
> > Cc: linux-omap@vger.kernel.org; Hiroshi Doyu; Ramirez Luna, Omar
> > Subject: RE: [PATCH 3/4] omap mailbox: remove mbox_configured scheme
> > 
> > 
> > > -----Original Message-----
> > > From: linux-omap-owner@vger.kernel.org 
> > > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Ohad 
> > Ben-Cohen
> > > Sent: Thursday, June 24, 2010 3:23 PM
> > > To: Kanigeri, Hari
> > > Cc: linux-omap@vger.kernel.org; Hiroshi Doyu; Ramirez Luna, Omar
> > > Subject: Re: [PATCH 3/4] omap mailbox: remove 
> mbox_configured scheme
> > > 
> > > On Thu, Jun 24, 2010 at 6:10 PM, Kanigeri, Hari 
> > <h-kanigeri2@ti.com> 
> > > wrote:
> > > > Ohad,
> > > >
> > > >> mbox_configured is global and therefore does not support
> > > concurrent
> > > >> usage of multiple mailbox instances.
> > > >
> > > > -- Why do you say that it doesn't support concurrent usage
> > > of multiple mailbox instances ? If you take example of 
> > OMAP4, we have 
> > > 2 mailbox instances, one talking to DSP and other to Ducati 
> > and they 
> > > should be supported concurrently.
> > > 
> > > 
> > > Interesting. was this actually tested and found working ?
> > > 
> > I think by supporting multiple instances, what Hari meant was 
> > that, for DSP and Ducati 2 different struct omap_mbox * are 
> > returned. They still can maintain their own callback function
> > (http://dev.omapzoom.org/?p=tisyslink/kernel-syslink.git;a=blo
> > b;f=arch/arm/mach-omap2/mailbox.c;h=a23c7edf1e84ab4fa51e56d0c2
> > daf2922084751a;hb=438f7a6a3cced478eb121426201206f6205fbbdc#l32
> > 7 for Ducati and
> > 
> > http://dev.omapzoom.org/?p=tisyslink/kernel-syslink.git;a=blob
> > ;f=arch/arm/mach-omap2/mailbox.c;h=a23c7edf1e84ab4fa51e56d0c2d
> > af2922084751a;hb=438f7a6a3cced478eb121426201206f6205fbbdc#l334
> > for DSP).
> > 
> > The "mbox_configured" was a means to reference count for one 
> > instance of the struct omap_mbox * .
> Sorry I that that back. Even though we maintain 2 strucutres, 
> only the one that calls the request_irq() is honoured. Hence 
> yes only one callback will be serviced (Thank to Ramesh for 
> pointing that out). Chaining of callbacks is a good way to 
> allow multiple clients to be notified.

I agree, from your other mail[1], I agree on the option 2, which would provide
support for multiple listeners for mailbox rx interrupt.

[1] http://marc.info/?l=linux-omap&m=127741171610403&w=2

Thanks
Ramesh Gupta G

RE: [PATCH 1/4] omap mailbox: extend API to take a callback param

[Kanigeri, Hari]

Ohad,

> 
> On Thu, Jun 24, 2010 at 6:02 PM, Kanigeri, Hari <h-kanigeri2@ti.com>
> wrote:
> > Looks good.
> 
> PCMIIW: from your other mail, I now understand that on OMAP4 you call
> omap_mbox_get several times to allow several concurrent mbox senders.

I don't know what PCMIIW stands for :). omap_mbox_get can be called by several concurrent mbox senders (1 sender per co-processor).

It looks like the variable mbox_configured that was added previously doesn't cover all the issues. It covers the shutdown issue with reference counting but as you mentioned that it poses an issue at startup as the RX interrupt for the second mbox instance doesn't get enabled.


> 
> If that's the case, this new API concept is a bit broken: it should
> allow several senders to call it, but will only support a single rx
> callback (probably by ignoring NULL callbacks, but that's not really
> safe).
> 
> We have two options here:
> 
> 1. do not extend omap_mbox_get to get a callback. instead, add a new
> API that explicitely sets the callback pointer.
> 
> 2. do extend omap_mbox_get to get a callback, but then employ
> something like a notifier chain of callbacks, essentially supporting
> multiple callbacks. this will allow a more flexible API, something
> that Hiroshi also wanted.

-- Even though this might not be relevant to mbox_configured issue, this looks like a nice feature to add. So this means we are going to support multiple writers to mailbox instance?

Thank you,
Best regards,
Hari

Re: [PATCH 1/4] omap mailbox: extend API to take a callback param

[Ohad Ben-Cohen]

On Fri, Jun 25, 2010 at 3:52 AM, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
> I don't know what PCMIIW stands for :)

please correct me if I'm wrong :)


> It looks like the variable mbox_configured that was added previously doesn't cover all the issues. It covers the shutdown issue with reference counting but as you mentioned that it poses an issue at startup as the RX interrupt for the second mbox instance doesn't get enabled.


yeah.


> -- Even though this might not be relevant to mbox_configured issue, this looks like a nice feature to add. So this means we are going to support multiple writers to mailbox instance?


yes, multiple writers, but also it will allow multiple readers.