On Wed, May 10, 2017 at 12:41 AM, Bjorn Andersson
[off-list ref] wrote:

On Tue 09 May 09:41 PDT 2017, Jassi Brar wrote:

quoted

quoted

quoted

quoted

quoted

The client should call mbox_client_txdone() after
mbox_send_message().

So every time we call mbox_send_message() from any of the client drivers
we also needs to call mbox_client_txdone()?

Yes.

quoted

This seems like an awkward side effect of using the mailbox framework -
which has to be spread out in at least 6 different client drivers :(

No. Mailbox or whatever you implement - you must (and do) tick the
state machine to keep the messages moving.

But the state you have in the other mailbox drivers is not a concern of
the APCS IPC.

No, as you say above you check for space before writing the next
message, this is what I call ticking the state machine.

Sure, but you're talking about the mailbox state machine. The APCS IPC
doesn't have states.  The _only_ thing that the APCS IPC provides is a
mechanism for informing the other side that "hey there's something to
do". So it doesn't matter if there's already a pending "hey there's
something to do", because adding another will still only be "hey there's
something to do".

I'm just trying to describe the big picture, but you keep confusing the
mailbox/doorbell responsibilities with the client's responsibilities.

I think I do understand the bigger picture...

The client driver sets up data packet in SHM and submit a "doorbell"
to be ringed. The controller driver simply sets some bit to trigger an
irq on the remote side (doorbell). And before submitting a "doorbell"
the client makes sure there is some space for data packet to be
written. Right?  You see, in the big picture you do have a
state-machine.

                 [Message to send]
                               |
                               |
           |-------------->|
           | No             |
           |                   |
           |___[Space Available?]
                               |
                               |Yes
                               |
                               |
                  [ Setup Data in SHM]
                               |
                              V
                    [Ring Doorbell]


Mailbox framework supports this whole picture. There is even a
callback (tx_prepare) to setup data packet just before the doorbell is
to be rung.

quoted

BTW, this is an option only if your client driver doesn't want to
explicitly tick the state machine by calling mbox_client_txdone()...
which I think should be done in the first place.

There is no state of the APCS IPC, so the overhead is created by the
mailbox framework.

Overhead remains the same if you move the check from your client
drivers to last_tx_done.
OR your client driver, rightfully, drive the state machine by calling
mbox_client_txdone() like other platforms.

                 [Message to send]<----------|
                               |                             |
                               |                             |
           |-------------->|                             |
           | No             |                             |
           |                   |                             |
           |___[Space Available?]             |
                               |                             |
                               |Yes                       |
                               |                             |
                              V                             |
                  [Setup Data in SHM]           |
                               |                              |
                              V                              |
                 mbox_send_message()        |
                               |                               |
                              V                              |
                 mbox_client_txdone()           |
                               |                              |
                               V______________|

The part where this piece of hardware differs from the other mailboxes
is that TX is done as send_data() returns and in the realm of the
mailbox there is no such thing as "tx done". So how about we extend the
framework to handle stateless and message-less doorbells?

This is a very common usecase. It would be unfair to other platforms
to modify the API just because you find it awkward to call
mbox_client_txdone() right after mbox_send_message(). For example,
drivers/firmware/tegra/bpmp.c
I'd much rather have mbox_send_message_and_tick() than implant a new api.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html