Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver

From: Jassi Brar <jassisinghbrar@gmail.com>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>,
	Andy Gross <andy.gross@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Ohad Ben-Cohen <ohad@wizery.com>,
	linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org,
	Devicetree List <devicetree@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-remoteproc@vger.kernel.org
Subject: Re: [PATCH v4 3/5] soc: qcom: Introduce APCS IPC driver
Date: Mon, 8 May 2017 12:17:02 +0530	[thread overview]
Message-ID: <CABb+yY3CfWFBDh-qoGHDeHyLhVC7mPQZFB=VDS54OaypG-BqPw@mail.gmail.com> (raw)
In-Reply-To: <20170508055439.GJ15143@minitux>

On Mon, May 8, 2017 at 11:24 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> 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.

> 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.

>> 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.
  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.

  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().