[PATCHv3 7/8] mailbox/omap: add code to support the wkupm3 operations

[PATCHv3 7/8] mailbox/omap: add code to support the wkupm3 operations

[Suman Anna]

The WkupM3 mailbox used for triggering PM operations such as suspend
and resume on AM33x/AM43x is special in that the M3 processor cannot
access the mailbox registers. However, an interrupt is needed to be
sent to request the M3 to perform a desired PM operation. This patch
adds the support for this special mailbox through separate ops for
this mailbox. These ops are designed to have the WkupM3's Rx interrupt
programmed within the driver, during transmission of a message. The
message is immediately read back and the internal mailbox interrupt
acknowledged as well to avoid triggering any spurious interrupts to
the M3.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/mailbox/mailbox-omap2.c | 87 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 86 insertions(+), 1 deletion(-)

diff --git a/drivers/mailbox/mailbox-omap2.c b/drivers/mailbox/mailbox-omap2.c
index fef18f4..00ac2a2 100644
--- a/drivers/mailbox/mailbox-omap2.c
+++ b/drivers/mailbox/mailbox-omap2.c
@@ -36,6 +36,8 @@
 #define MAILBOX_IRQ_NEWMSG(m)		(1 << (2 * (m)))
 #define MAILBOX_IRQ_NOTFULL(m)		(1 << (2 * (m) + 1))
 
+#define AM33X_MBOX_WKUPM3_USR		3
+
 #define MBOX_REG_SIZE			0x120
 
 #define OMAP4_MBOX_REG_SIZE		0x130
@@ -179,6 +181,57 @@ static int omap2_mbox_is_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq)
 	return (int)(enable & status & bit);
 }
 
+static void wkupm3_mbox_enable_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq)
+{
+	struct omap_mbox2_priv *p = mbox->priv;
+	u32 l, bit = (irq == IRQ_TX) ? p->notfull_bit : p->newmsg_bit;
+	unsigned long irqenable = ((irq == IRQ_RX) ?
+		OMAP4_MAILBOX_IRQENABLE(AM33X_MBOX_WKUPM3_USR) : p->irqenable);
+
+	l = mbox_read_reg(mbox->parent, irqenable);
+	l |= bit;
+	mbox_write_reg(mbox->parent, l, irqenable);
+}
+
+static void wkupm3_mbox_disable_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq)
+{
+	struct omap_mbox2_priv *p = mbox->priv;
+	u32 bit = (irq == IRQ_TX) ? p->notfull_bit : p->newmsg_bit;
+	unsigned long irqdisable = ((irq == IRQ_RX) ?
+	    OMAP4_MAILBOX_IRQENABLE_CLR(AM33X_MBOX_WKUPM3_USR) : p->irqdisable);
+
+	mbox_write_reg(mbox->parent, bit, irqdisable);
+}
+
+static void wkupm3_mbox_ack_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq)
+{
+	struct omap_mbox2_priv *p = mbox->priv;
+	u32 bit = (irq == IRQ_TX) ? p->notfull_bit : p->newmsg_bit;
+	unsigned long irqstatus = ((irq == IRQ_RX) ?
+		OMAP4_MAILBOX_IRQSTATUS(AM33X_MBOX_WKUPM3_USR) : p->irqstatus);
+
+	mbox_write_reg(mbox->parent, bit, irqstatus);
+
+	/* Flush posted write for irq status to avoid spurious interrupts */
+	mbox_read_reg(mbox->parent, irqstatus);
+}
+
+static int wkupm3_mbox_is_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq)
+{
+	struct omap_mbox2_priv *p = mbox->priv;
+	u32 bit = (irq == IRQ_TX) ? p->notfull_bit : p->newmsg_bit;
+	u32 enable, status;
+
+	/* WkupM3 mailbox does not use a receive queue */
+	if (irq == IRQ_RX)
+		return 0;
+
+	enable = mbox_read_reg(mbox->parent, p->irqenable);
+	status = mbox_read_reg(mbox->parent, p->irqstatus);
+
+	return (int)(enable & status & bit);
+}
+
 static void omap2_mbox_save_ctx(struct omap_mbox *mbox)
 {
 	int i;
@@ -215,6 +268,20 @@ static void omap2_mbox_restore_ctx(struct omap_mbox *mbox)
 	}
 }
 
+static void wkupm3_mbox_send_data(struct omap_mbox *mbox, mbox_msg_t msg)
+{
+	mbox_msg_t rmsg;
+
+	/* enable the mbox Rx interrupt for WkupM3 only briefly */
+	wkupm3_mbox_enable_irq(mbox, IRQ_RX);
+	omap2_mbox_fifo_write(mbox, msg);
+	wkupm3_mbox_disable_irq(mbox, IRQ_RX);
+
+	/* read back the message and ack the interrupt on behalf of WkupM3 */
+	rmsg = omap2_mbox_fifo_read(mbox);
+	wkupm3_mbox_ack_irq(mbox, IRQ_RX);
+}
+
 static struct omap_mbox_ops omap2_mbox_ops = {
 	.startup	= omap2_mbox_startup,
 	.shutdown	= omap2_mbox_shutdown,
@@ -230,6 +297,21 @@ static struct omap_mbox_ops omap2_mbox_ops = {
 	.restore_ctx	= omap2_mbox_restore_ctx,
 };
 
+static struct omap_mbox_ops wkupm3_mbox_ops = {
+	.startup	= omap2_mbox_startup,
+	.shutdown	= omap2_mbox_shutdown,
+	.fifo_read	= omap2_mbox_fifo_read,
+	.fifo_write	= wkupm3_mbox_send_data,
+	.fifo_empty	= omap2_mbox_fifo_empty,
+	.poll_for_space	= omap2_mbox_poll_for_space,
+	.enable_irq	= wkupm3_mbox_enable_irq,
+	.disable_irq	= wkupm3_mbox_disable_irq,
+	.ack_irq	= wkupm3_mbox_ack_irq,
+	.is_irq		= wkupm3_mbox_is_irq,
+	.save_ctx	= omap2_mbox_save_ctx,
+	.restore_ctx	= omap2_mbox_restore_ctx,
+};
+
 static const struct of_device_id omap_mailbox_of_match[] = {
 	{
 		.compatible	= "ti,omap2-mailbox",
@@ -387,7 +469,10 @@ static int omap2_mbox_probe(struct platform_device *pdev)
 		mbox->priv = priv;
 		mbox->parent = mdev;
 		mbox->name = info->name;
-		mbox->ops = &omap2_mbox_ops;
+		if (!strcmp(mbox->name, "wkup_m3"))
+			mbox->ops = &wkupm3_mbox_ops;
+		else
+			mbox->ops = &omap2_mbox_ops;
 		mbox->irq = platform_get_irq(pdev, info->irq_id);
 		if (mbox->irq < 0) {
 			ret = mbox->irq;
-- 
1.8.3.3

Re: [PATCHv3 7/8] mailbox/omap: add code to support the wkupm3 operations

[Kevin Hilman]

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

> The WkupM3 mailbox used for triggering PM operations such as suspend
> and resume on AM33x/AM43x is special in that the M3 processor cannot
> access the mailbox registers. However, an interrupt is needed to be
> sent to request the M3 to perform a desired PM operation. This patch
> adds the support for this special mailbox through separate ops for
> this mailbox. These ops are designed to have the WkupM3's Rx interrupt
> programmed within the driver, during transmission of a message. The
> message is immediately read back and the internal mailbox interrupt
> acknowledged as well to avoid triggering any spurious interrupts to
> the M3.
>
> Signed-off-by: Suman Anna <s-anna@ti.com>

Dumb Q: why does all this extra logic belong in the mailbox driver and
not in the wkup_m3 driver?  To me, this seems like part of the IPC
protocol between the MPU and M3 firmware, and not an inherent part of
the AM33xx mbox.

Kevin

Re: [PATCHv3 7/8] mailbox/omap: add code to support the wkupm3 operations

[Suman Anna]

Kevin,

On 08/26/2013 10:50 PM, Kevin Hilman wrote:
> Suman Anna <s-anna@ti.com> writes:
> 
>> The WkupM3 mailbox used for triggering PM operations such as suspend
>> and resume on AM33x/AM43x is special in that the M3 processor cannot
>> access the mailbox registers. However, an interrupt is needed to be
>> sent to request the M3 to perform a desired PM operation. This patch
>> adds the support for this special mailbox through separate ops for
>> this mailbox. These ops are designed to have the WkupM3's Rx interrupt
>> programmed within the driver, during transmission of a message. The
>> message is immediately read back and the internal mailbox interrupt
>> acknowledged as well to avoid triggering any spurious interrupts to
>> the M3.
>>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
> 
> Dumb Q: why does all this extra logic belong in the mailbox driver and
> not in the wkup_m3 driver?  To me, this seems like part of the IPC
> protocol between the MPU and M3 firmware, and not an inherent part of
> the AM33xx mbox.

The IPC protocol state machine for the PM operations is still very much
handled in the WkupM3 driver. The IPC registers in Control module
provide the messaging payloads, but unfortunately it is not associated
with any direct interrupts to make it a separate driver. As far as the
WkupM3 driver is concerned, it is just using the mailbox for signalling
- the internals of which would involve accessing the various mailbox
registers including interrupt configuration and clearing. It is
preferable to have the various operations on mailbox registers handled
by the mailbox driver with the API supporting the necessary operations.

The previous version from Vaibhav added a new API to mailbox driver for
clearing out the Tx fifo, which is non-standard. The mailboxes on AM335
will also be used for IPC with the Programmable Real-time Unit (PRU)
subsystem, which will use separate mbox devices. This version handles
the wkup_m3 mbox device using its device-specific ops without the need
for any new API, and not having to expose the mailbox registers outside.

regards
Suman

Re: [PATCHv3 7/8] mailbox/omap: add code to support the wkupm3 operations

[Kevin Hilman]

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

> Kevin,
>
> On 08/26/2013 10:50 PM, Kevin Hilman wrote:
>> Suman Anna <s-anna@ti.com> writes:
>> 
>>> The WkupM3 mailbox used for triggering PM operations such as suspend
>>> and resume on AM33x/AM43x is special in that the M3 processor cannot
>>> access the mailbox registers. However, an interrupt is needed to be
>>> sent to request the M3 to perform a desired PM operation. This patch
>>> adds the support for this special mailbox through separate ops for
>>> this mailbox. These ops are designed to have the WkupM3's Rx interrupt
>>> programmed within the driver, during transmission of a message. The
>>> message is immediately read back and the internal mailbox interrupt
>>> acknowledged as well to avoid triggering any spurious interrupts to
>>> the M3.

After reading this multiple times, I had to go back to the TRM to try
and remember what's going on here because this changelog wasn't helping
me understand.  IIUC, the basic idea is that the mailbox is only used to
trigger an IRQ to the M3, the messages themselves are dummy.  You
explained some more of this in subsequent replies, but all of that
detail should be in the changelogs.

Remember that changelogs should be written for those who don't have (or
don't remember) all the internal details that you know off the top of
your head.  Think of writing a changelog that you'll have to understand
in a year when you haven't been staring at the hardware and code.  I've
never seen a changelog with too much detail, so please err on the
verbose side.

>>>
>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> 
>> Dumb Q: why does all this extra logic belong in the mailbox driver and
>> not in the wkup_m3 driver?  To me, this seems like part of the IPC
>> protocol between the MPU and M3 firmware, and not an inherent part of
>> the AM33xx mbox.
>
> The IPC protocol state machine for the PM operations is still very
> much handled in the WkupM3 driver.  The IPC registers in Control
> module provide the messaging payloads, but unfortunately it is not
> associated with any direct interrupts to make it a separate driver. As
> far as the WkupM3 driver is concerned, it is just using the mailbox
> for signalling - the internals of which would involve accessing the
> various mailbox registers including interrupt configuration and
> clearing. It is preferable to have the various operations on mailbox
> registers handled by the mailbox driver with the API supporting the
> necessary operations.
>
> The previous version from Vaibhav added a new API to mailbox driver for
> clearing out the Tx fifo, which is non-standard. The mailboxes on AM335
> will also be used for IPC with the Programmable Real-time Unit (PRU)
> subsystem, which will use separate mbox devices. This version handles
> the wkup_m3 mbox device using its device-specific ops without the need
> for any new API, and not having to expose the mailbox registers outside.

Perhaps I'm misunderstanding all of this (admittedly, I'm not up on the
details of OMAP mailbox internals), but the changelog and code are not
helping me understand so I have to ask dumb questions.

The more I look at this, the more confused I get.  The wkupm3 mbox is
defined in the DTS to use usr_id=0 yet internally is
overridden/hard-coded internally to use usr_id=3 in (at least for
managing interrupts.)

Since only the interrupt management matters anyways (message payload is
dummy), is there any reason not to define the mbox in DTS using usr_id=3
so you don't have to do this clumsy override?  IOW, since the M3
(usr_id=3) can't ever be a real user of the mbox registers, just have
the MPU drive the mbox as usr_id=3?

Even that is a bit hacky IMO, but with proper documentation, is at least
better than the current approach IMO.

That should at least get rid of the need to customize the IRQ management
functions of mailbox-omap2.c.   After that, the M3 driver could then
just do:

     omap_mbox_enable_irq()
     omap_mbox_msg_send()
     omap_mbox_disable_irq()

and you could get rid of the rest of the custom functions in
mailbox-omap2.c also.

Kevin

Re: [PATCHv3 7/8] mailbox/omap: add code to support the wkupm3 operations

[Suman Anna]

Kevin,

On 08/27/2013 04:25 PM, Kevin Hilman wrote:
> Suman Anna <s-anna@ti.com> writes:
> 
>> Kevin,
>>
>> On 08/26/2013 10:50 PM, Kevin Hilman wrote:
>>> Suman Anna <s-anna@ti.com> writes:
>>>
>>>> The WkupM3 mailbox used for triggering PM operations such as suspend
>>>> and resume on AM33x/AM43x is special in that the M3 processor cannot
>>>> access the mailbox registers. However, an interrupt is needed to be
>>>> sent to request the M3 to perform a desired PM operation. This patch
>>>> adds the support for this special mailbox through separate ops for
>>>> this mailbox. These ops are designed to have the WkupM3's Rx interrupt
>>>> programmed within the driver, during transmission of a message. The
>>>> message is immediately read back and the internal mailbox interrupt
>>>> acknowledged as well to avoid triggering any spurious interrupts to
>>>> the M3.
> 
> After reading this multiple times, I had to go back to the TRM to try
> and remember what's going on here because this changelog wasn't helping
> me understand.  IIUC, the basic idea is that the mailbox is only used to
> trigger an IRQ to the M3, the messages themselves are dummy.  You
> explained some more of this in subsequent replies, but all of that
> detail should be in the changelogs.
> 
> Remember that changelogs should be written for those who don't have (or
> don't remember) all the internal details that you know off the top of
> your head.  Think of writing a changelog that you'll have to understand
> in a year when you haven't been staring at the hardware and code.  I've
> never seen a changelog with too much detail, so please err on the
> verbose side.
> 
>>>>
>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>>
>>> Dumb Q: why does all this extra logic belong in the mailbox driver and
>>> not in the wkup_m3 driver?  To me, this seems like part of the IPC
>>> protocol between the MPU and M3 firmware, and not an inherent part of
>>> the AM33xx mbox.
>>
>> The IPC protocol state machine for the PM operations is still very
>> much handled in the WkupM3 driver.  The IPC registers in Control
>> module provide the messaging payloads, but unfortunately it is not
>> associated with any direct interrupts to make it a separate driver. As
>> far as the WkupM3 driver is concerned, it is just using the mailbox
>> for signalling - the internals of which would involve accessing the
>> various mailbox registers including interrupt configuration and
>> clearing. It is preferable to have the various operations on mailbox
>> registers handled by the mailbox driver with the API supporting the
>> necessary operations.
>>
>> The previous version from Vaibhav added a new API to mailbox driver for
>> clearing out the Tx fifo, which is non-standard. The mailboxes on AM335
>> will also be used for IPC with the Programmable Real-time Unit (PRU)
>> subsystem, which will use separate mbox devices. This version handles
>> the wkup_m3 mbox device using its device-specific ops without the need
>> for any new API, and not having to expose the mailbox registers outside.
> 
> Perhaps I'm misunderstanding all of this (admittedly, I'm not up on the
> details of OMAP mailbox internals), but the changelog and code are not
> helping me understand so I have to ask dumb questions.
> 
> The more I look at this, the more confused I get.  The wkupm3 mbox is
> defined in the DTS to use usr_id=0 yet internally is
> overridden/hard-coded internally to use usr_id=3 in (at least for
> managing interrupts.)
>
> Since only the interrupt management matters anyways (message payload is
> dummy), is there any reason not to define the mbox in DTS using usr_id=3
> so you don't have to do this clumsy override?  IOW, since the M3
> (usr_id=3) can't ever be a real user of the mbox registers, just have
> the MPU drive the mbox as usr_id=3?

Yeah, the wkupm3 mbox usage is actually odd (an unfortunate result of
the way the hardware is designed) and doesn't quite fit into the way a
regular mailbox is used. I will summarize the following into the
changelog in the next version to better explain the odd usage and avoid
any confusion.

A typical mbox device is used for communicating both to and fro with a
slave processor using two fifos, and has two internal interrupts on the
MPU that needs to be handled - a rx interrupt for processing in-bound
messages and a tx-ready interrupt for handling mbox tx readiness. Since
the WkupM3 cannot access the mailbox registers, the burden of handling
the M3's Rx interrupt as well as those associated with the MPU lies with
the wkupm3 mbox device code. As you can see, this usage is non-standard,
and warrants that the code deal with both usr_ids since they would be
different (0 for MPU and 3 for M3). The OMAP mailbox core does use the
internal per-device ops, so it is actually better to plug-in custom ops
for WkupM3, and deal with both usr_ids within the ops. I had actually
started out with usr_id=3 but then I changed it to usr_id=0 so that it
aligns with the DT documentation and the expected behavior that usr_id
corresponds to the MPU, like for all mboxes on all other SoCs.

> 
> Even that is a bit hacky IMO, but with proper documentation, is at least
> better than the current approach IMO.

Even with approach you are suggesting, you still need to have specific
ops since the MPU interrupt handling code needs to deal with two usr_ids
for handling MPU Tx and WkupM3 Rx interrupt.

> 
> That should at least get rid of the need to customize the IRQ management
> functions of mailbox-omap2.c.   After that, the M3 driver could then
> just do:
> 
>      omap_mbox_enable_irq()
>      omap_mbox_msg_send()
>      omap_mbox_disable_irq()

This is infact how it was done in the previous AM335 PM suspend version
[1] along with another API to read-back the message put in the Tx queue
[2]. We do not want to add any new users of omap_mbox_enable_irq() or
omap_mbox_disable_irq() API, since these will not fit into the generic
mailbox framework. The actual IRQ raised in the M3's NVIC would be
handled by M3, so the entire functionality of enabling, disabling the
M3's mailbox Rx interrupt and reading back the message is handled in the
omap_mbox_msg_send() without having to expose any new additional API.

regards
Suman

> 
> and you could get rid of the rest of the custom functions in
> mailbox-omap2.c also.
>

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-November/129519.html
[2]
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-November/129504.html

Re: [PATCHv3 7/8] mailbox/omap: add code to support the wkupm3 operations

[Kevin Hilman]

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

[...]

> Yeah, the wkupm3 mbox usage is actually odd (an unfortunate result of
> the way the hardware is designed) 

heh, "design" is a generous term for this hardware. ;)

> and doesn't quite fit into the way a regular mailbox is used. I will
> summarize the following into the changelog in the next version to
> better explain the odd usage and avoid any confusion.
>
> A typical mbox device is used for communicating both to and fro with a
> slave processor using two fifos, and has two internal interrupts on the
> MPU that needs to be handled - a rx interrupt for processing in-bound
> messages

but there are no inbound message, so we don't care about that
interrupt.

> and a tx-ready interrupt for handling mbox tx readiness. 

Yes, this is where it gets tricky.  However, in the case of AM33xx, the
MPU doesn't care about RX interrupts and the M3 doesn't care about TX
interrupts.  hmm

> Since the WkupM3 cannot access the mailbox registers, the burden of
> handling the M3's Rx interrupt as well as those associated with the
> MPU lies with the wkupm3 mbox device code. As you can see, this usage
> is non-standard, and warrants that the code deal with both usr_ids
> since they would be different (0 for MPU and 3 for M3). 

Another possibility would be to allow configurability over which usr_id
is used for RX and which usr_id is used for TX.  IOW, have the DT
binding have a usr_id_tx and and optional usr_id_rx.  If usr_id_rx isn't
present (for most normal users), it uses usr_id_tx.  For AM33xx, we'd
use usr_id_tx=0, usr_id_rx=3.

That would allow you to get rid of the overrides for the IRQ functions,
no?  But as I think about it, this id a bit yucky too.

Yet another possiblity would be to use the DT to define 2 mailboxes.
One "normal" one for control of the MPU's view (usr_id=0) and one dummy
one (usr_id=3) for control of the M3's view of the world.  Since Linux
needs to control both, just define them both in the DT for linux
control, and drive them from the M3 driver:

  mbox_mpu = omap_mbox_get(<normal one>)
  mbox_m3 = omap_mbox_get(<M3 one>)

  omap_mbox_enable_irq(mbox_m3, IRQ_RX)
  omap_mbox_msg_send(mbox_mpu, msg)
  omap_mbox_disable_irq(mbox_m3, IRQ_RX)
  omap_mbox_fifo_readback(mbox_mpu) /* API from earlier patch */  
  omap_mbox_ack_irq(mbox_m3) /* this would need to be added/exported */

AFAICS, other than the new APIs to export, this wouldn't require any
additional M3 quirks in the mailbox driver. 

To me, what's important here is that the quirkiness of the M3 setup
remains contained in the M3 driver (where it should be), not hidden in
the mailbox driver, when it has nothing to do with the mailbox hardware.

> The OMAP mailbox core does use the internal per-device ops, so it is
> actually better to plug-in custom ops for WkupM3, and deal with both
> usr_ids within the ops. 

This is where I disagree.  The M3 brokenness should be contained in the
M3 driver, not the mailbox driver.

> I had actually started out with usr_id=3 but
> then I changed it to usr_id=0 so that it aligns with the DT
> documentation 

Which DT documentation?  I don't see how the DT documentation forces you
into any usr_id choice.

> and the expected behavior that usr_id corresponds to the MPU, like for
> all mboxes on all other SoCs.

For AM33xx, I think you can throw out any ideas of "expected
behavior". ;) Using usr_id=3 as described above along with some
documentation in the .dts would go a long ways.

>> 
>> Even that is a bit hacky IMO, but with proper documentation, is at least
>> better than the current approach IMO.
>
> Even with approach you are suggesting, you still need to have specific
> ops since the MPU interrupt handling code needs to deal with two usr_ids
> for handling MPU Tx and WkupM3 Rx interrupt.

Not if you actually define 2 mailboxes in the DT and use them both from
the driver as I describe above.

>> 
>> That should at least get rid of the need to customize the IRQ management
>> functions of mailbox-omap2.c.   After that, the M3 driver could then
>> just do:
>> 
>>      omap_mbox_enable_irq()
>>      omap_mbox_msg_send()
>>      omap_mbox_disable_irq()
>
> This is infact how it was done in the previous AM335 PM suspend version
> [1] along with another API to read-back the message put in the Tx queue
> [2]. 

Except that the previous version was doing the readback/flush in the
txev_handler of the M3 driver instead of in the message send like the
current version (and my proposed one above.)

> We do not want to add any new users of omap_mbox_enable_irq() or
> omap_mbox_disable_irq() API, since these will not fit into the generic
> mailbox framework. 

What generic mailbox framework?  The only mailbox API I see currently is
omap_mbox_*.  Assuming there's a generic one coming, how will a generic
mailbox framework work without the enable/disable IRQ?  What other
method would there be for determining the receiver of the message?
Whatever that new generic API is for that is, it would have to use the
omap_mbox_[enable|disable]_irq() APIs internally, so they are those not
going away, so I don't see why you can't add more users.  IOW, any users of
omap_mbox_[enable|disable]_irq() would be converted to the new API for
configuring the message receiver.

> The actual IRQ raised in the M3's NVIC would be
> handled by M3, so the entire functionality of enabling, disabling the
> M3's mailbox Rx interrupt and reading back the message is handled in the
> omap_mbox_msg_send() without having to expose any new additional API.

So the fundamental difference we have here is that you believe exposing
a new API is wrong and I believe hiding non-mailbox related quirks of
the M3 inside the mailbox driver is wrong.

I've tried to argue above why I think adding the API is not a big deal.
At least, it's not as big a deal (to me) as hiding M3 quirks inside the
mailbox driver.

Kevin

Re: [PATCHv3 7/8] mailbox/omap: add code to support the wkupm3 operations

[Suman Anna]

Kevin,

Sorry, I couldn't get back earlier due to long weekend.

On 08/29/2013 11:57 AM, Kevin Hilman wrote:
> Suman Anna <s-anna@ti.com> writes:
> 
> [...]
> 
>> Yeah, the wkupm3 mbox usage is actually odd (an unfortunate result of
>> the way the hardware is designed) 
> 
> heh, "design" is a generous term for this hardware. ;)
> 
>> and doesn't quite fit into the way a regular mailbox is used. I will
>> summarize the following into the changelog in the next version to
>> better explain the odd usage and avoid any confusion.
>>
>> A typical mbox device is used for communicating both to and fro with a
>> slave processor using two fifos, and has two internal interrupts on the
>> MPU that needs to be handled - a rx interrupt for processing in-bound
>> messages
> 
> but there are no inbound message, so we don't care about that
> interrupt.
> 
>> and a tx-ready interrupt for handling mbox tx readiness. 
> 
> Yes, this is where it gets tricky.  However, in the case of AM33xx, the
> MPU doesn't care about RX interrupts and the M3 doesn't care about TX
> interrupts.  hmm

Correct, MPU still needs to care about the Tx interrupt though.

> 
>> Since the WkupM3 cannot access the mailbox registers, the burden of
>> handling the M3's Rx interrupt as well as those associated with the
>> MPU lies with the wkupm3 mbox device code. As you can see, this usage
>> is non-standard, and warrants that the code deal with both usr_ids
>> since they would be different (0 for MPU and 3 for M3). 
> 
> Another possibility would be to allow configurability over which usr_id
> is used for RX and which usr_id is used for TX.  IOW, have the DT
> binding have a usr_id_tx and and optional usr_id_rx.  If usr_id_rx isn't
> present (for most normal users), it uses usr_id_tx.  For AM33xx, we'd
> use usr_id_tx=0, usr_id_rx=3.
> 
> That would allow you to get rid of the overrides for the IRQ functions,
> no?  But as I think about it, this id a bit yucky too.
> 
> Yet another possiblity would be to use the DT to define 2 mailboxes.
> One "normal" one for control of the MPU's view (usr_id=0) and one dummy
> one (usr_id=3) for control of the M3's view of the world.  Since Linux
> needs to control both, just define them both in the DT for linux
> control, and drive them from the M3 driver:

I like the idea of having the ability to define a omap mailbox as
rx-only or tx-only or both, rather than defining two usr_ids. So far,
for all the IPC communication between processors, we always needed a
pair since we only had a single instance and the fifos are
unidirectional. For DRA7xx, where we have multiple instances, this
should allow flexibility to pick different fifos from different
instances. I will redo the patches to support this feature in general.

> 
>   mbox_mpu = omap_mbox_get(<normal one>)
>   mbox_m3 = omap_mbox_get(<M3 one>)
> 
>   omap_mbox_enable_irq(mbox_m3, IRQ_RX)
>   omap_mbox_msg_send(mbox_mpu, msg)
>   omap_mbox_disable_irq(mbox_m3, IRQ_RX)
>   omap_mbox_fifo_readback(mbox_mpu) /* API from earlier patch */  
>   omap_mbox_ack_irq(mbox_m3) /* this would need to be added/exported */

If you look at this sequence, there's an inherent understanding of the
mailbox behavior. The enable and disable are used to suppress additional
interrupts, since the mailbox IP would always trigger an interrupt as
long as there is a message within the FIFO. The fifo_readback is
clearing the message, but it need not be done in the response message as
was done in the previous PM series. The WkupM3 driver has to primarily
just trigger an interrupt.

> 
> AFAICS, other than the new APIs to export, this wouldn't require any
> additional M3 quirks in the mailbox driver. 

The omap_mbox_fifo_readback is not a generic mailbox API and actually
makes the mailbox driver complex since the driver does support tx
buffering as well. This is the case irrespective of the framework. The
omap_mbox_ack_irq is also not generic, since the irq management usually
lies with the mailbox driver, and the clients need not be bothered.

> 
> To me, what's important here is that the quirkiness of the M3 setup
> remains contained in the M3 driver (where it should be), not hidden in
> the mailbox driver, when it has nothing to do with the mailbox hardware.
> 
>> The OMAP mailbox core does use the internal per-device ops, so it is
>> actually better to plug-in custom ops for WkupM3, and deal with both
>> usr_ids within the ops. 
> 
> This is where I disagree.  The M3 brokenness should be contained in the
> M3 driver, not the mailbox driver.
> 
>> I had actually started out with usr_id=3 but
>> then I changed it to usr_id=0 so that it aligns with the DT
>> documentation 
> 
> Which DT documentation?

Documentation in Patch 2 [3] of this series, where I add the DT binding
documentation and the guidelines on what each field means.

>  I don't see how the DT documentation forces you into any usr_id choice.
> 
>> and the expected behavior that usr_id corresponds to the MPU, like for
>> all mboxes on all other SoCs.
> 
> For AM33xx, I think you can throw out any ideas of "expected
> behavior". ;) 

Yep, seems like a common theme indeed ;).

> Using usr_id=3 as described above along with some
> documentation in the .dts would go a long ways.
> 
>>>
>>> Even that is a bit hacky IMO, but with proper documentation, is at least
>>> better than the current approach IMO.
>>
>> Even with approach you are suggesting, you still need to have specific
>> ops since the MPU interrupt handling code needs to deal with two usr_ids
>> for handling MPU Tx and WkupM3 Rx interrupt.
> 
> Not if you actually define 2 mailboxes in the DT and use them both from
> the driver as I describe above.
> 
>>>
>>> That should at least get rid of the need to customize the IRQ management
>>> functions of mailbox-omap2.c.   After that, the M3 driver could then
>>> just do:
>>>
>>>      omap_mbox_enable_irq()
>>>      omap_mbox_msg_send()
>>>      omap_mbox_disable_irq()
>>
>> This is infact how it was done in the previous AM335 PM suspend version
>> [1] along with another API to read-back the message put in the Tx queue
>> [2]. 
> 
> Except that the previous version was doing the readback/flush in the
> txev_handler of the M3 driver instead of in the message send like the
> current version (and my proposed one above.)
> 
>> We do not want to add any new users of omap_mbox_enable_irq() or
>> omap_mbox_disable_irq() API, since these will not fit into the generic
>> mailbox framework. 
> 
> What generic mailbox framework?  The only mailbox API I see currently is
> omap_mbox_*.  

Yeah, there's a generic one still under development [4]. You can look
through the mailbox_client.h for the generic API that would be used by a
client. I started out the support for AM335 mailbox on top of it
originally, the preliminary dev work including the OMAP adaptation is
hosted at [5] (using usr_id 3) & [6] (using usr_id 0), but decided to
post this to break the dependency and unblock the PM series. I will
adapt all existing OMAP mbox clients at once the framework shows up.

> Assuming there's a generic one coming, how will a generic
> mailbox framework work without the enable/disable IRQ?  

The IRQ handling would be internal to the driver, the clients would
not be required to manage them. The framework provides API for the
clients to send messages, and receive callbacks for rx messages.

> What other method would there be for determining the receiver of the message?

A client would operate on a mbox device, so the knowledge of receiver is
automatic.

> Whatever that new generic API is for that is, it would have to use the
> omap_mbox_[enable|disable]_irq() APIs internally, so they are those not
> going away, so I don't see why you can't add more users.

At present, DSP/Bridge is the only user of this API. The remoteproc
infrastructure doesn't use the omap_mbox_[enable|disable]_irq(), and we
want all clients to use the generic API in the long term.

> IOW, any users of
> omap_mbox_[enable|disable]_irq() would be converted to the new API for
> configuring the message receiver.

For now, these would be exposed until I rework the DSP/Bridge to remove
these.

> 
>> The actual IRQ raised in the M3's NVIC would be
>> handled by M3, so the entire functionality of enabling, disabling the
>> M3's mailbox Rx interrupt and reading back the message is handled in the
>> omap_mbox_msg_send() without having to expose any new additional API.
> 
> So the fundamental difference we have here is that you believe exposing
> a new API is wrong and I believe hiding non-mailbox related quirks of
> the M3 inside the mailbox driver is wrong.

Yeah, the mailbox IP is external to both the MPU and the WkupM3 IPs,
with a dedicated mailbox driver dealing with the IP. It is definitely
debatable as to who should be responsible for the AM335 PM usecase. The
WkupM3 driver is currently using mailbox to raise the interrupt, and if
it were using a different interrupt trigger source, then it would have
to use that respective driver's API. The WkupM3 usage of mailbox is odd,
no matter how we look at it.

My primary reasoning is that we want all clients to migrate to the
generic API, so we do not want to be adding/exposing (at this point)
driver-specific API for dealing with quirks. My approach was to deal
with the wkupm3 interaction/quirks as a device-specific ops within the
driver for this reason.

regards
Suman

[3] http://marc.info/?l=linux-omap&m=137582562717980&w=2
[4] http://marc.info/?l=linux-kernel&m=136782510209473&w=2
[5]
https://github.com/sumananna/mailbox/commits/jassiv3-3.11rc1-am335-mbox-suspend
[6]
https://github.com/sumananna/mailbox/commits/jassiv3-3.11rc3-am335-mbox-suspend-v1

Re: [PATCHv3 7/8] mailbox/omap: add code to support the wkupm3 operations

[Kevin Hilman]

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

> Kevin,
>
> Sorry, I couldn't get back earlier due to long weekend.
>
> On 08/29/2013 11:57 AM, Kevin Hilman wrote:
>> Suman Anna <s-anna@ti.com> writes:
>> 
>> [...]
>> 
>>> Yeah, the wkupm3 mbox usage is actually odd (an unfortunate result of
>>> the way the hardware is designed) 
>> 
>> heh, "design" is a generous term for this hardware. ;)
>> 
>>> and doesn't quite fit into the way a regular mailbox is used. I will
>>> summarize the following into the changelog in the next version to
>>> better explain the odd usage and avoid any confusion.
>>>
>>> A typical mbox device is used for communicating both to and fro with a
>>> slave processor using two fifos, and has two internal interrupts on the
>>> MPU that needs to be handled - a rx interrupt for processing in-bound
>>> messages
>> 
>> but there are no inbound message, so we don't care about that
>> interrupt.
>> 
>>> and a tx-ready interrupt for handling mbox tx readiness. 
>> 
>> Yes, this is where it gets tricky.  However, in the case of AM33xx, the
>> MPU doesn't care about RX interrupts and the M3 doesn't care about TX
>> interrupts.  hmm
>
> Correct, MPU still needs to care about the Tx interrupt though.
>
>> 
>>> Since the WkupM3 cannot access the mailbox registers, the burden of
>>> handling the M3's Rx interrupt as well as those associated with the
>>> MPU lies with the wkupm3 mbox device code. As you can see, this usage
>>> is non-standard, and warrants that the code deal with both usr_ids
>>> since they would be different (0 for MPU and 3 for M3). 
>> 
>> Another possibility would be to allow configurability over which usr_id
>> is used for RX and which usr_id is used for TX.  IOW, have the DT
>> binding have a usr_id_tx and and optional usr_id_rx.  If usr_id_rx isn't
>> present (for most normal users), it uses usr_id_tx.  For AM33xx, we'd
>> use usr_id_tx=0, usr_id_rx=3.
>> 
>> That would allow you to get rid of the overrides for the IRQ functions,
>> no?  But as I think about it, this id a bit yucky too.
>> 
>> Yet another possiblity would be to use the DT to define 2 mailboxes.
>> One "normal" one for control of the MPU's view (usr_id=0) and one dummy
>> one (usr_id=3) for control of the M3's view of the world.  Since Linux
>> needs to control both, just define them both in the DT for linux
>> control, and drive them from the M3 driver:
>
> I like the idea of having the ability to define a omap mailbox as
> rx-only or tx-only or both, rather than defining two usr_ids. So far,
> for all the IPC communication between processors, we always needed a
> pair since we only had a single instance and the fifos are
> unidirectional. For DRA7xx, where we have multiple instances, this
> should allow flexibility to pick different fifos from different
> instances. I will redo the patches to support this feature in general.

Sounds good.

Does that mean you plan to define a TX only mailbox for the MPU view
(usr_id=0) and an RX only mailbox for the M3 view (usr_id=3)?


>> 
>>   mbox_mpu = omap_mbox_get(<normal one>)
>>   mbox_m3 = omap_mbox_get(<M3 one>)
>> 
>>   omap_mbox_enable_irq(mbox_m3, IRQ_RX)
>>   omap_mbox_msg_send(mbox_mpu, msg)
>>   omap_mbox_disable_irq(mbox_m3, IRQ_RX)
>>   omap_mbox_fifo_readback(mbox_mpu) /* API from earlier patch */  
>>   omap_mbox_ack_irq(mbox_m3) /* this would need to be added/exported */
>
> If you look at this sequence, there's an inherent understanding of the
> mailbox behavior. The enable and disable are used to suppress additional
> interrupts, since the mailbox IP would always trigger an interrupt as
> long as there is a message within the FIFO. The fifo_readback is
> clearing the message, but it need not be done in the response message as
> was done in the previous PM series. The WkupM3 driver has to primarily
> just trigger an interrupt.

OK, I didn't fully understand the need for the readback everytime
either, but was just following what was proposed in this series.

>> 
>> AFAICS, other than the new APIs to export, this wouldn't require any
>> additional M3 quirks in the mailbox driver. 
>
> The omap_mbox_fifo_readback is not a generic mailbox API and actually
> makes the mailbox driver complex since the driver does support tx
> buffering as well. 

I think this could be solved by just having some quirks flags or similar
for each mbox device.  e.g. a quirk that says "read back message
immediately after sending" or something like that.  Or maybe an "IRQ
only" mode that implies the readback to keep the fifo empty but still
triggers the IRQ.

> This is the case irrespective of the framework. The
> omap_mbox_ack_irq is also not generic, since the irq management usually
> lies with the mailbox driver, and the clients need not be bothered.

Similarily, you could have another "quirk" flag such that the
disable_irq also does an ack.  Or maybe it should already do this?

>> 
>> To me, what's important here is that the quirkiness of the M3 setup
>> remains contained in the M3 driver (where it should be), not hidden in
>> the mailbox driver, when it has nothing to do with the mailbox hardware.
>> 
>>> The OMAP mailbox core does use the internal per-device ops, so it is
>>> actually better to plug-in custom ops for WkupM3, and deal with both
>>> usr_ids within the ops. 
>> 
>> This is where I disagree.  The M3 brokenness should be contained in the
>> M3 driver, not the mailbox driver.
>> 
>>> I had actually started out with usr_id=3 but
>>> then I changed it to usr_id=0 so that it aligns with the DT
>>> documentation 
>> 
>> Which DT documentation?
>
> Documentation in Patch 2 [3] of this series, where I add the DT binding
> documentation and the guidelines on what each field means.
>
>>  I don't see how the DT documentation forces you into any usr_id choice.
>> 
>>> and the expected behavior that usr_id corresponds to the MPU, like for
>>> all mboxes on all other SoCs.
>> 
>> For AM33xx, I think you can throw out any ideas of "expected
>> behavior". ;) 
>
> Yep, seems like a common theme indeed ;).
>
>> Using usr_id=3 as described above along with some
>> documentation in the .dts would go a long ways.
>> 
>>>>
>>>> Even that is a bit hacky IMO, but with proper documentation, is at least
>>>> better than the current approach IMO.
>>>
>>> Even with approach you are suggesting, you still need to have specific
>>> ops since the MPU interrupt handling code needs to deal with two usr_ids
>>> for handling MPU Tx and WkupM3 Rx interrupt.
>> 
>> Not if you actually define 2 mailboxes in the DT and use them both from
>> the driver as I describe above.
>> 
>>>>
>>>> That should at least get rid of the need to customize the IRQ management
>>>> functions of mailbox-omap2.c.   After that, the M3 driver could then
>>>> just do:
>>>>
>>>>      omap_mbox_enable_irq()
>>>>      omap_mbox_msg_send()
>>>>      omap_mbox_disable_irq()
>>>
>>> This is infact how it was done in the previous AM335 PM suspend version
>>> [1] along with another API to read-back the message put in the Tx queue
>>> [2]. 
>> 
>> Except that the previous version was doing the readback/flush in the
>> txev_handler of the M3 driver instead of in the message send like the
>> current version (and my proposed one above.)
>> 
>>> We do not want to add any new users of omap_mbox_enable_irq() or
>>> omap_mbox_disable_irq() API, since these will not fit into the generic
>>> mailbox framework. 
>> 
>> What generic mailbox framework?  The only mailbox API I see currently is
>> omap_mbox_*.  
>
> Yeah, there's a generic one still under development [4]. You can look
> through the mailbox_client.h for the generic API that would be used by a
> client. I started out the support for AM335 mailbox on top of it
> originally, the preliminary dev work including the OMAP adaptation is
> hosted at [5] (using usr_id 3) & [6] (using usr_id 0), but decided to
> post this to break the dependency and unblock the PM series. I will
> adapt all existing OMAP mbox clients at once the framework shows up.
>
>> Assuming there's a generic one coming, how will a generic
>> mailbox framework work without the enable/disable IRQ?  
>
> The IRQ handling would be internal to the driver, the clients would
> not be required to manage them. The framework provides API for the
> clients to send messages, and receive callbacks for rx messages.
>
>> What other method would there be for determining the receiver of the message?
>
> A client would operate on a mbox device, so the knowledge of receiver is
> automatic.
>
>> Whatever that new generic API is for that is, it would have to use the
>> omap_mbox_[enable|disable]_irq() APIs internally, so they are those not
>> going away, so I don't see why you can't add more users.
>
> At present, DSP/Bridge is the only user of this API. The remoteproc
> infrastructure doesn't use the omap_mbox_[enable|disable]_irq(), and we
> want all clients to use the generic API in the long term.

OK, then I think the only solution is to add some quirks/modes to the
generic code so it can handle the case where client has to manage both
the sender and the receiver.

>> IOW, any users of
>> omap_mbox_[enable|disable]_irq() would be converted to the new API for
>> configuring the message receiver.
>
> For now, these would be exposed until I rework the DSP/Bridge to remove
> these.
>
>> 
>>> The actual IRQ raised in the M3's NVIC would be
>>> handled by M3, so the entire functionality of enabling, disabling the
>>> M3's mailbox Rx interrupt and reading back the message is handled in the
>>> omap_mbox_msg_send() without having to expose any new additional API.
>> 
>> So the fundamental difference we have here is that you believe exposing
>> a new API is wrong and I believe hiding non-mailbox related quirks of
>> the M3 inside the mailbox driver is wrong.
>
> Yeah, the mailbox IP is external to both the MPU and the WkupM3 IPs,
> with a dedicated mailbox driver dealing with the IP. It is definitely
> debatable as to who should be responsible for the AM335 PM usecase. The
> WkupM3 driver is currently using mailbox to raise the interrupt, and if
> it were using a different interrupt trigger source, then it would have
> to use that respective driver's API. The WkupM3 usage of mailbox is odd,
> no matter how we look at it.

Precisely.

IIUC, AM43xx has the ability to trigger an interrupt by a write to the
control module, which eliminates the need for using the mailbox hardware
all together.  Is that correct?

> My primary reasoning is that we want all clients to migrate to the
> generic API, so we do not want to be adding/exposing (at this point)
> driver-specific API for dealing with quirks. My approach was to deal
> with the wkupm3 interaction/quirks as a device-specific ops within the
> driver for this reason.

I agree that's the right motiviation.  However if a "generic" driver
doesn't handle existing uses on existing hardware, then it's probably
not generic enough.

Could you explore adding some more general quirks/modes to the generic
driver such that it could handle the AM335x brokenness^Wpeculiarity.

Kevin

Re: [PATCHv3 7/8] mailbox/omap: add code to support the wkupm3 operations

[Suman Anna]

Kevin,

>>
>> Sorry, I couldn't get back earlier due to long weekend.
>>
>> On 08/29/2013 11:57 AM, Kevin Hilman wrote:
>>> Suman Anna <s-anna@ti.com> writes:
>>>
>>> [...]
>>>
>>>> Yeah, the wkupm3 mbox usage is actually odd (an unfortunate result of
>>>> the way the hardware is designed) 
>>>
>>> heh, "design" is a generous term for this hardware. ;)
>>>
>>>> and doesn't quite fit into the way a regular mailbox is used. I will
>>>> summarize the following into the changelog in the next version to
>>>> better explain the odd usage and avoid any confusion.
>>>>
>>>> A typical mbox device is used for communicating both to and fro with a
>>>> slave processor using two fifos, and has two internal interrupts on the
>>>> MPU that needs to be handled - a rx interrupt for processing in-bound
>>>> messages
>>>
>>> but there are no inbound message, so we don't care about that
>>> interrupt.
>>>
>>>> and a tx-ready interrupt for handling mbox tx readiness. 
>>>
>>> Yes, this is where it gets tricky.  However, in the case of AM33xx, the
>>> MPU doesn't care about RX interrupts and the M3 doesn't care about TX
>>> interrupts.  hmm
>>
>> Correct, MPU still needs to care about the Tx interrupt though.
>>
>>>
>>>> Since the WkupM3 cannot access the mailbox registers, the burden of
>>>> handling the M3's Rx interrupt as well as those associated with the
>>>> MPU lies with the wkupm3 mbox device code. As you can see, this usage
>>>> is non-standard, and warrants that the code deal with both usr_ids
>>>> since they would be different (0 for MPU and 3 for M3). 
>>>
>>> Another possibility would be to allow configurability over which usr_id
>>> is used for RX and which usr_id is used for TX.  IOW, have the DT
>>> binding have a usr_id_tx and and optional usr_id_rx.  If usr_id_rx isn't
>>> present (for most normal users), it uses usr_id_tx.  For AM33xx, we'd
>>> use usr_id_tx=0, usr_id_rx=3.
>>>
>>> That would allow you to get rid of the overrides for the IRQ functions,
>>> no?  But as I think about it, this id a bit yucky too.
>>>
>>> Yet another possiblity would be to use the DT to define 2 mailboxes.
>>> One "normal" one for control of the MPU's view (usr_id=0) and one dummy
>>> one (usr_id=3) for control of the M3's view of the world.  Since Linux
>>> needs to control both, just define them both in the DT for linux
>>> control, and drive them from the M3 driver:
>>
>> I like the idea of having the ability to define a omap mailbox as
>> rx-only or tx-only or both, rather than defining two usr_ids. So far,
>> for all the IPC communication between processors, we always needed a
>> pair since we only had a single instance and the fifos are
>> unidirectional. For DRA7xx, where we have multiple instances, this
>> should allow flexibility to pick different fifos from different
>> instances. I will redo the patches to support this feature in general.
> 
> Sounds good.
> 
> Does that mean you plan to define a TX only mailbox for the MPU view
> (usr_id=0) and an RX only mailbox for the M3 view (usr_id=3)?

I will see how this pans out for the M3 view, because mainly from the
API point of view, we will have one to send, and the receive is usually
a callback.

> 
>>>
>>>   mbox_mpu = omap_mbox_get(<normal one>)
>>>   mbox_m3 = omap_mbox_get(<M3 one>)
>>>
>>>   omap_mbox_enable_irq(mbox_m3, IRQ_RX)
>>>   omap_mbox_msg_send(mbox_mpu, msg)
>>>   omap_mbox_disable_irq(mbox_m3, IRQ_RX)
>>>   omap_mbox_fifo_readback(mbox_mpu) /* API from earlier patch */  
>>>   omap_mbox_ack_irq(mbox_m3) /* this would need to be added/exported */
>>
>> If you look at this sequence, there's an inherent understanding of the
>> mailbox behavior. The enable and disable are used to suppress additional
>> interrupts, since the mailbox IP would always trigger an interrupt as
>> long as there is a message within the FIFO. The fifo_readback is
>> clearing the message, but it need not be done in the response message as
>> was done in the previous PM series. The WkupM3 driver has to primarily
>> just trigger an interrupt.
> 
> OK, I didn't fully understand the need for the readback everytime
> either, but was just following what was proposed in this series.

Yeah, the reads are needed to take out the message, since the h/w has a
fifo, and as long as messages are present (number doesn't matter) and
mbox irq enabled, the interrupt keeps getting triggered. Reading the
message, and acking the interrupt within the mailbox IP are the typical
actions performed on an mbox interrupt. This patch essentially was doing
all the above steps with the send message API, so at the end of it, an
interrupt is raised within the M3's NVIC, and the firmware deals with
performing the necessary steps based on the message in IPC Control
registers.

> 
>>>
>>> AFAICS, other than the new APIs to export, this wouldn't require any
>>> additional M3 quirks in the mailbox driver. 
>>
>> The omap_mbox_fifo_readback is not a generic mailbox API and actually
>> makes the mailbox driver complex since the driver does support tx
>> buffering as well. 
> 
> I think this could be solved by just having some quirks flags or similar
> for each mbox device.  e.g. a quirk that says "read back message
> immediately after sending" or something like that.  Or maybe an "IRQ
> only" mode that implies the readback to keep the fifo empty but still
> triggers the IRQ.
> 
>> This is the case irrespective of the framework. The
>> omap_mbox_ack_irq is also not generic, since the irq management usually
>> lies with the mailbox driver, and the clients need not be bothered.
> 
> Similarily, you could have another "quirk" flag such that the
> disable_irq also does an ack.  Or maybe it should already do this?

Yeah, I will look into if a quirk flag can be plumbed into regular
function ops. My initial development was deciding the quirk based on an
string compare, and it was much simpler to do this decision at the time
of plugging in the ops, so that other SoCs are not at all affected by
having to check for the quirk.

> 
>>>
>>> To me, what's important here is that the quirkiness of the M3 setup
>>> remains contained in the M3 driver (where it should be), not hidden in
>>> the mailbox driver, when it has nothing to do with the mailbox hardware.
>>>
>>>> The OMAP mailbox core does use the internal per-device ops, so it is
>>>> actually better to plug-in custom ops for WkupM3, and deal with both
>>>> usr_ids within the ops. 
>>>
>>> This is where I disagree.  The M3 brokenness should be contained in the
>>> M3 driver, not the mailbox driver.
>>>
>>>> I had actually started out with usr_id=3 but
>>>> then I changed it to usr_id=0 so that it aligns with the DT
>>>> documentation 
>>>
>>> Which DT documentation?
>>
>> Documentation in Patch 2 [3] of this series, where I add the DT binding
>> documentation and the guidelines on what each field means.
>>
>>>  I don't see how the DT documentation forces you into any usr_id choice.
>>>
>>>> and the expected behavior that usr_id corresponds to the MPU, like for
>>>> all mboxes on all other SoCs.
>>>
>>> For AM33xx, I think you can throw out any ideas of "expected
>>> behavior". ;) 
>>
>> Yep, seems like a common theme indeed ;).
>>
>>> Using usr_id=3 as described above along with some
>>> documentation in the .dts would go a long ways.
>>>
>>>>>
>>>>> Even that is a bit hacky IMO, but with proper documentation, is at least
>>>>> better than the current approach IMO.
>>>>
>>>> Even with approach you are suggesting, you still need to have specific
>>>> ops since the MPU interrupt handling code needs to deal with two usr_ids
>>>> for handling MPU Tx and WkupM3 Rx interrupt.
>>>
>>> Not if you actually define 2 mailboxes in the DT and use them both from
>>> the driver as I describe above.
>>>
>>>>>
>>>>> That should at least get rid of the need to customize the IRQ management
>>>>> functions of mailbox-omap2.c.   After that, the M3 driver could then
>>>>> just do:
>>>>>
>>>>>      omap_mbox_enable_irq()
>>>>>      omap_mbox_msg_send()
>>>>>      omap_mbox_disable_irq()
>>>>
>>>> This is infact how it was done in the previous AM335 PM suspend version
>>>> [1] along with another API to read-back the message put in the Tx queue
>>>> [2]. 
>>>
>>> Except that the previous version was doing the readback/flush in the
>>> txev_handler of the M3 driver instead of in the message send like the
>>> current version (and my proposed one above.)
>>>
>>>> We do not want to add any new users of omap_mbox_enable_irq() or
>>>> omap_mbox_disable_irq() API, since these will not fit into the generic
>>>> mailbox framework. 
>>>
>>> What generic mailbox framework?  The only mailbox API I see currently is
>>> omap_mbox_*.  
>>
>> Yeah, there's a generic one still under development [4]. You can look
>> through the mailbox_client.h for the generic API that would be used by a
>> client. I started out the support for AM335 mailbox on top of it
>> originally, the preliminary dev work including the OMAP adaptation is
>> hosted at [5] (using usr_id 3) & [6] (using usr_id 0), but decided to
>> post this to break the dependency and unblock the PM series. I will
>> adapt all existing OMAP mbox clients at once the framework shows up.
>>
>>> Assuming there's a generic one coming, how will a generic
>>> mailbox framework work without the enable/disable IRQ?  
>>
>> The IRQ handling would be internal to the driver, the clients would
>> not be required to manage them. The framework provides API for the
>> clients to send messages, and receive callbacks for rx messages.
>>
>>> What other method would there be for determining the receiver of the message?
>>
>> A client would operate on a mbox device, so the knowledge of receiver is
>> automatic.
>>
>>> Whatever that new generic API is for that is, it would have to use the
>>> omap_mbox_[enable|disable]_irq() APIs internally, so they are those not
>>> going away, so I don't see why you can't add more users.
>>
>> At present, DSP/Bridge is the only user of this API. The remoteproc
>> infrastructure doesn't use the omap_mbox_[enable|disable]_irq(), and we
>> want all clients to use the generic API in the long term.
> 
> OK, then I think the only solution is to add some quirks/modes to the
> generic code so it can handle the case where client has to manage both
> the sender and the receiver.
> 
>>> IOW, any users of
>>> omap_mbox_[enable|disable]_irq() would be converted to the new API for
>>> configuring the message receiver.
>>
>> For now, these would be exposed until I rework the DSP/Bridge to remove
>> these.
>>
>>>
>>>> The actual IRQ raised in the M3's NVIC would be
>>>> handled by M3, so the entire functionality of enabling, disabling the
>>>> M3's mailbox Rx interrupt and reading back the message is handled in the
>>>> omap_mbox_msg_send() without having to expose any new additional API.
>>>
>>> So the fundamental difference we have here is that you believe exposing
>>> a new API is wrong and I believe hiding non-mailbox related quirks of
>>> the M3 inside the mailbox driver is wrong.
>>
>> Yeah, the mailbox IP is external to both the MPU and the WkupM3 IPs,
>> with a dedicated mailbox driver dealing with the IP. It is definitely
>> debatable as to who should be responsible for the AM335 PM usecase. The
>> WkupM3 driver is currently using mailbox to raise the interrupt, and if
>> it were using a different interrupt trigger source, then it would have
>> to use that respective driver's API. The WkupM3 usage of mailbox is odd,
>> no matter how we look at it.
> 
> Precisely.
> 
> IIUC, AM43xx has the ability to trigger an interrupt by a write to the
> control module, which eliminates the need for using the mailbox hardware
> all together.  Is that correct?

I am not sure, can't make out clearly from the early TRM drafts. But it
would be great, if there is one. If the way the M3 interrupts the MPU is
also a generic interrupt, then the IPC registers along with these two
interrupts could be made into its own mailbox driver (would expect it to
look something similar to the current pl320-ipc driver).

> 
>> My primary reasoning is that we want all clients to migrate to the
>> generic API, so we do not want to be adding/exposing (at this point)
>> driver-specific API for dealing with quirks. My approach was to deal
>> with the wkupm3 interaction/quirks as a device-specific ops within the
>> driver for this reason.
> 
> I agree that's the right motiviation.  However if a "generic" driver
> doesn't handle existing uses on existing hardware, then it's probably
> not generic enough.
> 
> Could you explore adding some more general quirks/modes to the generic
> driver such that it could handle the AM335x brokenness^Wpeculiarity.

The quirks would have to be handled within the OMAP mailbox driver, I
doubt the generic core needs to know about these, but definitely I will
explore this as I rework my patches.

regards
Suman