[PATCH v8 02/14] ASoC: Intel: catpt: Implement IPC protocol

[Rojewski, Cezary]

On 2020-09-23 3:17 PM, Andy Shevchenko wrote:
> On Wed, Sep 23, 2020 at 02:24:56PM +0200, Cezary Rojewski wrote:
>> Implement IRQ handlers for immediate and delayed replies and
>> notifications. Communication is synchronous and allows for serialization
>> of maximum one message at a time.
>>
>> DSP may respond with ADSP_PENDING status for a request - known as
>> delayed reply - and when situation occurs, framework keeps the lock and
>> awaits upcoming response through IPCD channel which is handled in
>> bottom-half. Immediate replies spawn no BH at all as their processing is
>> very short.
> 
> ...
> 
>> +static void catpt_dsp_send_tx(struct catpt_dev *cdev,
>> +			      const struct catpt_ipc_msg *tx)
>> +{
>> +	u32 header = tx->header | CATPT_IPCC_BUSY;
> 
>> +	if (tx->size)
> 
> This check is not needed.
> 

It's really the battle between: avoiding memcpy(size=0) and size-check.
In the past, reviewers favored the former.

Ack.

>> +		memcpy_toio(catpt_outbox_addr(cdev), tx->data, tx->size);
>> +	catpt_writel_shim(cdev, IPCC, header);
>> +}
> 
> ...
> 
>> +	ret = ipc->rx.rsp.status;
>> +	if (reply) {
> 
>> +		reply->header = ipc->rx.header;
> 
> So, even in case of error the header is still updated?
> 

As per protocol, it is paramount to save the reply header regardless of
IPC status as long as communication with DSP is not severed AKA timeout.
Header contains actual error code provided by DSP firmware. While
existing catpt's implementation isn't granular in regard to error codes,
nothing stops the developer to do so and react differently depending on
the value of error code received.

Without storing the header, caller is blind about the severity of an
error.

>> +		if (!ret && reply->data && reply->size)
> 
>> +			memcpy(reply->data, ipc->rx.data, ipc->rx.size);
> 
> This I didn't get. You copy data by using source size?!
> 
>> +	}
>> +
>> +	return ret;
> 
> I guess the above piece may be refactored, but I don't know how until it is
> clear why it's written like this.
> 
> ...
> 

Well, ipc->rx.size equals reply->size as long as IPC procedure is in
progress and reply != NULL. So either ipc->rx.size or reply->size
suffices.

IPC protocol is quite simple here (albeit cAVS arguably is even simpler):

OUTBOX <channel: request - reply model>
TX: copy request header to appropriate register, copy payload to outbox
<IRQ>
RX: copy reply header, copy returned payload only in successful case

INBOX <channel: notification model>
No TX: no direct requests coming from HOST
RX: notifications from DSP, same as RX for OUTBOX applies here

Reply header is always yielded for possibly granular error handling.
Payload is omitted if SUCCESS is not the value of msg status.

Before the message is sent (catpt_dsp_do_send_msg()), it is validated
against maximum payload size and ipc status fields are re-initialized -
catpt_ipc_msg_init(). From them on, msg completion is awaited.

If we are dealing with standard IPC, reply should arrive no later than
within 300ms (in vast majority of cases, this won't reach 30ms) and be
found within outbox's RX. In case of heavy IPCs e.g.: reset
offload-stream, reply (ack) is yielded immediately from DSP, no
processing is done between and instead, DSP delegates the heavy task for
later. Response for that heavy task completion should arrive no latter
than 300ms from the moment initial reply (ack) was received and this
time, inbox's RX is used instead.

So, since catpt_dsp_copy_rx() knows nothing about caller's reply
(pointer to store the received data into), rx.size is assigned to
reply->size during the initialization phase so it can be reused during
that very moment - copying rx as there is no need to access SRAM if no
payload is expected.

>> +static void catpt_dsp_copy_rx(struct catpt_dev *cdev, u32 header)
>> +{
>> +	struct catpt_ipc *ipc = &cdev->ipc;
>> +
>> +	ipc->rx.header = header;
>> +	if (ipc->rx.size && ipc->rx.rsp.status == CATPT_REPLY_SUCCESS) {
> 
> Check for size is redundant.
> 
>> +		memcpy_fromio(ipc->rx.data, catpt_outbox_addr(cdev),
>> +			      ipc->rx.size);
>> +	}
> 
> Can be done like
> 
> 	if (status != SUCCESS)
> 		return;
> 	memcpy_fromio(...);
> 
>> +}
> 

Ack for both. The reduced indentation is very good suggestion, thanks!

Czarek