Suman Anna [off-list ref] writes:

Kevin,

On 08/26/2013 10:50 PM, Kevin Hilman wrote:

quoted

Suman Anna [off-list ref] writes:

quoted

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.

quoted

quoted

Signed-off-by: Suman Anna <redacted>

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