On Wed, May 10, 2017 at 12:41 AM, Bjorn Andersson
<bjorn.andersson@xxxxxxxxxx> wrote:
> On Tue 09 May 09:41 PDT 2017, Jassi Brar wrote:
>>
>> >> >> 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.
>> >>
>> >> > 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.
>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
