On Sun 07 May 23:47 PDT 2017, Jassi Brar wrote:

On Mon, May 8, 2017 at 11:24 AM, Bjorn Andersson
[off-list ref] wrote:

quoted

On Fri 05 May 21:48 PDT 2017, Jassi Brar wrote:

The APCS IPC register serves the basis for all inter-processor
communication in a Qualcomm platform, so it's not only the RPM driver
discussed earlier that uses this. It's also used for other non-FIFO
based communication channels, where the signalled information either
isn't acked at all or acked on a system-level.

Something has to indicate consumption of data or "requested action
taken". Otherwise the protocol is design-wise broken.

The SMD and GLINK protocols work by providing two independent one-way
pipes that higher levels can use to send and receive messages. When some
driver pushes a message into the transmit-pipe we check if there's
space, then write the message, signal the remote (APCS IPC) and then
return.

It's then up to the higher level driver and protocol what to do next. In
some cases it will expect that there will appear a packet on the
incoming pipe indicating that the action was taken, but there's nothing
in the communication path enforcing this.

By this you can have strictly one-way notifications, two-way
stop-and-wait style communication or multiple-messages-in-flight
communication over the same transport mechanism.


The remote will update the read index (in shared memory) as it consumes
the data from the FIFO, but under normal circumstances there are no
reason for it to actively notify the sender or for the sender to wait
for it to be consumed.

quoted

But regardless of the protocol implemented ontop, the purpose of the
APCS IPC bit is _only_ to invoke some remote handler to consume some
data, somewhere - the event in itself does not carry any information.

Yes, every platform that uses shared-memory works like that. However
there is always something that tells if the command has been acted
upon by the remote. In your case that is the read-pointer movement.

In a straight forward stop-and-wait flow control based setup like the
version of RPM previously discussed this makes a lot of sense. But there
are a multitude of different protocols using this mechanism to signal
that something has happened.

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.

  Best designs have some interrupt occurring when the message has been
consumed by the remote. Some designs have a flag set which needs to be
polled to detect completion. Very few (like yours) that support
neither irq nor polling, have to be driven by the upper protocol layer
by some ack packet (or tracking read/write pointers like you do).
These three cases are denoted by TXDONE_BY_IRQ, TXDONE_BY_POLL and
TXDONE_BY_ACK respectively.

You're confusing the APCS IPC with the larger communication mechanism,
flow control is taken care of in some higher layer - if it's needed at
all.

This is why I suggested that this is a doorbell, rather than a mailbox.
Your argumentation of how a mailbox should work makes perfect sense, but
it's not how the Qualcomm IPC works.

  If no client driver will ever submit a message if there is no space
in FIFO, then you can specify TXDONE_BY_POLL and have last_tx_done()
always return true. That way you don't need to call
mbox_client_txdone().

Clients of the APCS IPC will never post a message to the mailbox, it's
non-blocking and the "transaction" is done when the operation returns.
All the other parts of a "non-broken protocol" is a concern of some
other part of the software stack.


Setting TXDONE_BY_POLL and specifying a dummy last_tx_done() comes with
a crazy overhead. To set a single bit in a register we will take the
channel spinlock 4 times, start a timer, iterate over all registered
channels and the client must be marked as blocking so we will get at
least 2 additional context switches.

Regards,
Bjorn